[JIT] Cleanup, share more code between x86 and x64 in genAllocLclFrame

[TIHan]

Description
I'm in the process of getting familiar with our xarch codegen/emitter. Am trying to wrap my head around the differences between x86 and x64 for stack pointers - noticed that the two could share a little bit more logic in genAllocLclFrame.

This change is purely cleanup and should produce zero diffs.

Acceptance Criteria

• No Diffs

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description
I'm in the process of getting familiar with our xarch codegen/emitter. Am trying to wrap my head around the differences between x86 and x64 for stack pointers - noticed that the two could share a little bit more logic in genAllocLclFrame.

This change is purely cleanup and should produce zero diffs.

Acceptance Criteria

• No Diffs

Author:	TIHan
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[TIHan]

@dotnet/jit-contrib is ready @BruceForstall PTAL

[BruceForstall]

Hmmm... While this reduces a little code duplication, I don't think it helps code readability -- you've gone from 3 #ifdef/#else/#endif lines to 7. It seems like the logic flow is thus more complicated to understand.

[TIHan]

@BruceForstall - I do agree it seems a little more complicated, but on the other hand it's clearer to know what the differences are between x86/x64. I will close the PR as I don't think much else could be done.

[markples]

I think you can potentially iterate on this. A typical way to simplify the code (could be used with or without your PR) is to add abstractions for the complexity - likely function calls in this case. The key point is probably whether those function calls can have good abstraction value ((DoXXX, code, DoYYY) vs (Helper1, code, Helper2)). You've basically regrouped the code by moving it around - does this make it easier or harder to add good abstractions? For example, is the original large TARGET_X86 a good functional unit, do the new separate ones work better, or is it somewhere in between?

Looking at your PR code, one could ask

• first ifdef - maybe make a helper PublishStubParam() or ConsiderStubParam()?
• there's a section about stack probes. maybe that (even the code that is always the same) can be factored out?
• but why is that initReg stuff in the middle of the stack probe - does it make sense it move it out so the stack probe stuff is one block?
• oh wait, there's more with stub param, so PublishStubParam() above isn't good enough. Ok, how do the two sections relate - is there good naming that can reflect that?

The ideal would be that you can get this function to basically straight-line code that can be reasoned about without looking at the helpers. Then the helpers can individually be read. Then it's simpler. If you have to read the helpers' implementations in order to understand the main code, then it's more complicated.

Please note that this isn't a prescription for how to rewrite the code but the beginning of a thought process for how you could continue. It should raise more questions than it answers. And at some point, one of them could be "is the cleanup worth the effort?"

[TIHan]

You've basically regrouped the code by moving it around - does this make it easier or harder to add good abstractions?

Part of the issue is that I do not fully understand what was written, so it's hard to make a judgement call around building an abstraction. I based the cleanup only by looking at what was the same. I could create functions that can wrap the specific functionality, but my primary goal was being able to see the differences between the archs.

I didn't want to spend more time on this cleanup, but I will try to look at it again - so I'll re-open it.

[BruceForstall]

@TIHan Please change this to "Draft" (or close) if you aren't actively working on it.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.