[PATCH] D69034: [Alignment] Use Align for TFI.getStackAlignment() in X86ISelLowering

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.

In D69034#1725662 <https://reviews.llvm.org/D69034#1725662>, @craig.topper wrote:

> @rnk do you know much about this code?


Not previously, but from looking at it, I'd say the simplification is fine. This code is only called from the guaranteed TCO codepath, so I'm not surprised it's lightly tested.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:4186
 unsigned
-X86TargetLowering::GetAlignedArgumentStackSize(unsigned StackSize,
-                                               SelectionDAG& DAG) const {
-  const X86RegisterInfo *RegInfo = Subtarget.getRegisterInfo();
-  const TargetFrameLowering &TFI = *Subtarget.getFrameLowering();
-  unsigned StackAlignment = TFI.getStackAlignment();
-  uint64_t AlignMask = StackAlignment - 1;
-  int64_t Offset = StackSize;
-  unsigned SlotSize = RegInfo->getSlotSize();
-  if ( (Offset & AlignMask) <= (StackAlignment - SlotSize) ) {
-    // Number smaller than 12 so just add the difference.
-    Offset += ((StackAlignment - SlotSize) - (Offset & AlignMask));
-  } else {
-    // Mask out lower bits, add stackalignment once plus the 12 bytes.
-    Offset = ((~AlignMask) & Offset) + StackAlignment +
-      (StackAlignment-SlotSize);
-  }
-  return Offset;
+X86TargetLowering::GetAlignedArgumentStackSize(const unsigned StackSize,
+                                               SelectionDAG &DAG) const {
----------------
gchatelet wrote:
> courbet wrote:
> > This is not the same as the previous code for all values: https://godbolt.org/z/QTxj9U
> > Could you at least add an assert if you don;t intend to cover the same range of inputs ?
> I traced the addition of this code back to rL42870 (2007). Since the author is probably not working on this any more I'm redirecting to @craig.topper (from `CODE_OWNERS.TXT`).
> 
> The original code can be rewritten as:
> ```
> unsigned GetAlignedArgumentStackSize(uint64_t StackSize, uint64_t StackAlignment, uint64_t SlotSize) {
>   const uint64_t AlignMask = StackAlignment - 1;
>   const uint64_t lowBits = StackSize & AlignMask;
>   const uint64_t hiBits = StackSize & ~AlignMask;
>   if (lowBits <= (StackAlignment - SlotSize))
>     return StackSize - lowBits + StackAlignment - SlotSize;
>   return StackAlignment + hiBits + StackAlignment - SlotSize;
> }
> ```
> 
> I believe the last line is wrong, at least I can't wrap my head around it.
> interestingly, the last line is **never** exercised in the tests so I believe it is dead code.
> 
> As such I believe the patch cannot be `NFC` anymore.
I think the code only differs when StackAlignment is less than SlotSize, which should never happen in practice. I suspect we rely on that invariant in other places. I edited the godbolt program to avoid such test cases and now the algorithms always produce the same values: https://godbolt.org/z/MqPErL

And, the only way to exercise the second case in the original code would be if StackSize is not a multiple of SlotSize, which seems unlikely. I suspect we align StackSize to SlotSize somewhere else. If you wanted to add test coverage for this case, I'd experiment with multiple one byte allocas.

In the end, I think it's safe to take your simplification.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69034/new/

https://reviews.llvm.org/D69034