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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked 2 inline comments as done.
gchatelet added inline comments.


================
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 {
----------------
rnk wrote:
> 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.
Thx a lot @rnk . I've added an assert to make the invariant clear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69034