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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked 4 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 {
----------------
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.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22218
       SP = DAG.getNode(ISD::AND, dl, VT, SP.getValue(0),
-                       DAG.getConstant(-(uint64_t)Align, dl, VT));
+                       DAG.getConstant(~(Alignment->value() - 1ULL), dl, VT));
       Chain = DAG.getCopyToReg(Chain, dl, SPReg, SP);
----------------
courbet wrote:
> Let's refactor this in `Align` (in a separate patch).
SGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69034