This is an archive of the discontinued LLVM Phabricator instance.

[Alignment][NFC] Convert MachineIRBuilder::buildDynStackAlloc to Align
ClosedPublic

Authored by gchatelet on Apr 2 2020, 2:17 AM.

Details

Summary

The change in IRTranslator is not trivial but is NFC as far as I can tell.

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.Apr 2 2020, 2:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2020, 2:17 AM
courbet added inline comments.Apr 2 2020, 3:51 AM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1889

I think using a ternary would be more readable.

1895

I'd like to get a knowledgeable pair of eye to look at this. We used to align anything smaller than the stack size to 1, now we align to max(AI.align, preftypealign). This sounds OK, but let's be on the safe side.

gchatelet marked an inline comment as done.Apr 2 2020, 4:44 AM
gchatelet added a subscriber: aemerson.
gchatelet added inline comments.
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1895

@aemerson what do you think?

gchatelet marked an inline comment as done.Apr 2 2020, 4:57 AM
gchatelet added inline comments.
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1895

AFAICT it is not clear what it means use G_DYN_STACKALLOC with an alignment of 0
https://llvm.org/docs/GlobalISel/GenericOpcode.html#g-dyn-stackalloc

@aemerson can you help me understand what's going on here? Or add relevant people to the thread?

aemerson added inline comments.Apr 2 2020, 10:19 AM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1895

An alignment of 0 I believe is used to mean there is no alignment requirement at all.

gchatelet marked 2 inline comments as done.Apr 3 2020, 1:15 AM
gchatelet added inline comments.
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1895

Ok I dug into it and 0 or 1 have the same meaning.
https://reviews.llvm.org/D77372

For code simplicity, I'll use 1 here.

gchatelet updated this revision to Diff 254727.Apr 3 2020, 1:44 AM
gchatelet marked an inline comment as done.
  • Address comments and rebase
courbet requested changes to this revision.Apr 3 2020, 1:50 AM
courbet added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
381

What about taking an alignment now that we're clarified the meaning of 0 ? There's only one caller now and no alignment does not have any particular meaning compared to 1.

This revision now requires changes to proceed.Apr 3 2020, 1:50 AM
gchatelet updated this revision to Diff 254730.Apr 3 2020, 1:53 AM
  • Using Align instead of MaybeAlign
courbet accepted this revision.Apr 3 2020, 2:03 AM
This revision is now accepted and ready to land.Apr 3 2020, 2:03 AM
This revision was automatically updated to reflect the committed changes.