Page MenuHomePhabricator

[Alignment][NFC] Use TargetFrameLowering::getStackAlign()
ClosedPublic

Authored by gchatelet on Mar 21 2020, 1:41 PM.

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.Mar 21 2020, 1:41 PM
gchatelet updated this revision to Diff 251858.Mar 21 2020, 1:58 PM
gchatelet marked an inline comment as done.
  • inline variable
gchatelet added inline comments.Mar 21 2020, 1:58 PM
llvm/lib/Target/X86/X86InstrInfo.cpp
5376

Documentation here is useful to understand why this is correct.

[Alignment][NFC] Use TFL::getStackAlign()

Suggest: TFL -> TargetFrameLowering. Abbreviations on the title line can only cause confusion.

courbet added inline comments.Mar 23 2020, 1:43 AM
llvm/lib/CodeGen/PrologEpilogInserter.cpp
441

inline this on the line below ?

Alignment = std::min(Alignment, TFI->getStackAlign());

llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
132

const, to make it clear that this does not change in the loop ?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4031

I think it was clearer with this conditional here rather than inline later.

4043

extract StackAlign.value() - 1 to a StackAlignMask variable for semantics ? Or maybe even add a getMask() method to llvm::Align given that this is a very common operation ?

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
548

would be useful here too.

573

and here

[Alignment][NFC] Use TFL::getStackAlign()

Suggest: TFL -> TargetFrameLowering. Abbreviations on the title line can only cause confusion.

Will do, thx!

gchatelet retitled this revision from [Alignment][NFC] Use TFL::getStackAlign() to [Alignment][NFC] Use TargetFrameLowering::getStackAlign().Mar 23 2020, 2:16 AM
gchatelet marked 9 inline comments as done.Mar 23 2020, 2:44 AM
gchatelet added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4031

Unfortunately if I do so I have to use a MaybeAlign instead of Align to express the 0 case.
Is this better?

4043

I plan to do so as a separate step, maybe introducing a Mask type. There are less cases than it seems though so I'm still weighing the costs / benefits.

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
548

Acknowledged

gchatelet updated this revision to Diff 251969.Mar 23 2020, 2:44 AM
gchatelet marked 3 inline comments as done.
  • inline variable
  • Address comments
Herald added a reviewer: herhut. · View Herald Transcript
Herald added a reviewer: rriddle. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
gchatelet removed projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project.
gchatelet removed subscribers: jholewinski, emaste, jvesely and 57 others.
courbet accepted this revision.Mar 23 2020, 3:20 AM
courbet added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4031

I think so.

This revision is now accepted and ready to land.Mar 23 2020, 3:20 AM
Harbormaster failed remote builds in B50082: Diff 251969!
This revision was automatically updated to reflect the committed changes.