Page MenuHomePhabricator

[Alignment] Use Align for TFI.getStackAlignment() in X86ISelLowering
ClosedPublic

Authored by gchatelet on Oct 16 2019, 6:35 AM.

Details

Summary

This is patch is part of a series to introduce an Alignment type.
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html
See this patch for the introduction of the type: https://reviews.llvm.org/D64790

Diff Detail

Event Timeline

gchatelet created this revision.Oct 16 2019, 6:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2019, 6:36 AM
courbet requested changes to this revision.Oct 16 2019, 7:01 AM
courbet added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
4186–4192

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 ?

22264

Let's refactor this in Align (in a separate patch).

This revision now requires changes to proceed.Oct 16 2019, 7:01 AM
gchatelet marked 4 inline comments as done.Oct 18 2019, 7:15 AM
gchatelet added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
4186–4192

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.

22264

SGTM

gchatelet retitled this revision from [Alignment][NFC] Use Align for TFI.getStackAlignment() in X86ISelLowering to [Alignment] Use Align for TFI.getStackAlignment() in X86ISelLowering.Oct 18 2019, 7:40 AM
gchatelet marked 3 inline comments as done.Fri, Oct 25, 1:52 PM
craig.topper added a subscriber: rnk.

@rnk do you know much about this code?

rnk accepted this revision.Tue, Oct 29, 12:00 PM

@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.

llvm/lib/Target/X86/X86ISelLowering.cpp
4186–4192

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.

gchatelet updated this revision to Diff 227051.Wed, Oct 30, 2:21 AM
  • Add an assert to make the invariant clear
gchatelet marked 2 inline comments as done.Wed, Oct 30, 2:24 AM
gchatelet added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
4186–4192

Thx a lot @rnk . I've added an assert to make the invariant clear.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Oct 30, 2:40 AM
This revision was automatically updated to reflect the committed changes.
gchatelet marked an inline comment as done.