[PATCH] D11249: Fix PR#24142

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:664
@@ -663,2 +663,3 @@
   int SPAdj = MI->getOperand(0).getImm();
+  SPAdj = TFI->alignSPAdjust(SPAdj);
 
----------------
rnk wrote:
> mkuper wrote:
> > rnk wrote:
> > > I don't think this is the right place to fix this. getSPAdjust is also used on X86 to return SP adjustment values for PUSH, POP, CALL, and others. These typically produce values that are not a multiple of stack alignment (4 / 8 vs 16).
> > I think this actually is the right place for it.
> > This looks like a long-standing bug from before I factored this code out of PEI and into TFI.
> > 
> > From my commit message on the refactoring, emphasis is new:
> > "PEI tries to keep track of how much starting or ending a call sequence adjusts the stack pointer by, so that it can resolve frame-index references. Currently, it takes a very simplistic view of how SP adjustments are done - both FrameStartOpcode and FrameDestroyOpcode adjust it exactly by the amount written in its first argument.
> > 
> > This view is in fact incorrect for some targets (*e.g. due to stack re-alignment*, or because it may want to adjust the stack pointer in multiple steps). However, that doesn't cause breakage, because most targets (the only in-tree exception appears to be 32-bit ARM) rely on being able to simplify the call frame pseudo-instructions earlier, so this code is never hit. "
> > 
> > The newly-created (in a later commit) X86 version of getSPAdjust actually does take alignment into account, but, unfortunately, I never went back and fixed the ARM version - I didn't know it's broken in practice.
> Do you think the new behavior of aligning the outgoing call argument space should live in the ARM backend or the generic Target layer?
> 
> I'm surprised that the backends aren't responsible for building aligned call stack adjustments. I guess this ties into the whole crazy phase ordering of stack realignment, where we try to support late stack realignment if the register allocator wants it.
The generic target layer makes sense to me. The fact the call stack adjustment instructions don't take alignment into account isn't backend-specific.
I should probably also refactor the X86 code to call into the generic layer for the framesetup/framedestroy case. Will try to do that once this patch goes in.

And yes, I was surprised at that either.


http://reviews.llvm.org/D11249