This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Emit alignment "Max Skip" operand for align directives
ClosedPublic

Authored by NickGuy on Nov 25 2021, 6:39 AM.

Details

Summary

The current AsmPrinter has support to emit the "Max Skip" operand
(the 3rd of .p2align), however has no support for it to actually be specified.
Adding MaxBytesForAlignment to MachineBasicBlock provides this capability on a
per-block basis. Leaving the value as default (0) causes no observable differences
in behaviour.

Diff Detail

Event Timeline

NickGuy created this revision.Nov 25 2021, 6:39 AM
NickGuy requested review of this revision.Nov 25 2021, 6:39 AM
NickGuy retitled this revision from [CodeGen] Emit alignment "Max Skip" operand to [CodeGen] Emit alignment "Max Skip" operand for align directives.

Alignments are used in a lot of places, where you wouldn't really need to specify a maximum bytes added parameter, that's mostly use in aligning code sections as far as I understand. Do we really want to add this to the base Align class? Or should it just be part of whatever controls the code alignment?

Do we really want to add this to the base Align class? Or should it just be part of whatever controls the code alignment?

As far as I can tell, AsmPrinter::emitAlignment is what controls the code alignment. Beyond that point, all the parameters are either unsigned or int64_t types. I was hesitant to add it to Align, but this was the least-invasive solution I could come up with.

gchatelet requested changes to this revision.Nov 25 2021, 6:59 AM

Align is a very generic class used in many contexts. Enriching it with MaxBytesForAlignment will break the mental model of everyone using it.
If you need the constraint for the AsmPrinter then pass it along manually.

This revision now requires changes to proceed.Nov 25 2021, 6:59 AM
NickGuy updated this revision to Diff 390968.Dec 1 2021, 2:58 AM
NickGuy edited the summary of this revision. (Show Details)

Do you have a follow up patch in the pipeline that uses this? Or in other words, how could this be tested?

SjoerdMeijer added inline comments.Dec 1 2021, 3:07 AM
llvm/include/llvm/CodeGen/MachineBasicBlock.h
18

Some comments/explanations what this exactly would be good, like the other members here.

Not sure what the maximum value can be, if there is any, and if we can or should use a smaller int type to reduce the size of MBBs.

I've got a follow-up patch nearly ready with tests, yep. I want to settle on an approach here before implementing the other side of it.

I've got a follow-up patch nearly ready with tests, yep. I want to settle on an approach here before implementing the other side of it.

Okay, that's fine. Perhaps upload your WIP patches too then, stack them, so that we can actually see the (full) approach?

NickGuy updated this revision to Diff 391026.Dec 1 2021, 7:48 AM

Okay, that's fine. Perhaps upload your WIP patches too then, stack them, so that we can actually see the (full) approach?

Done, D114879 has been added as a child revision. I couldn't figure out how to mark it as a draft revision though, so I've left it with no reviewers for now.

It may be better to fold this into D114879 so that there is a single patch that can be tested properly.

llvm/include/llvm/CodeGen/AsmPrinter.h
1

Can you make sure you clang-format the changes.

llvm/include/llvm/CodeGen/MachineBasicBlock.h
20

1<<Alignment or 2^Alignment. It may be better to use the wording from elsewhere and say "If the alignment cannot be reached in this many bytes, no bytes are emitted."

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1

What about this code path?

courbet added inline comments.Dec 3 2021, 12:28 AM
llvm/include/llvm/CodeGen/MachineBasicBlock.h
22

Is there any reason for the inconsistency between unsigned in emitAlignment and uint8_t here ? I don't think this struct is particularly memory-constrained, so I would just use unsigned here.

NickGuy updated this revision to Diff 391624.Dec 3 2021, 6:36 AM
NickGuy marked 4 inline comments as done.

It may be better to fold this into D114879 so that there is a single patch that can be tested properly.

I've partially folded D114879 into this, I want to keep the AArch64-specific stuff separate though.

llvm/include/llvm/CodeGen/MachineBasicBlock.h
20

Right, Alignment^2 is not the same as 2^Alignment... Fixed

22

No particular reason beyond memory saving, if that's not a concern here then unsigned makes more sense.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1

I haven't seen anything take this path, but I've now added MaxBytesToEmit to this call too, following the logic that if MaxBytesToEmit is provided, it should be used.

Can you add a test to show that the alignment when emitting an object file is correct too?

I've partially folded D114879 into this, I want to keep the AArch64-specific stuff separate though.

OK. I'm not sure I understand why. It is easier to review patches that are complete and do something specific :)

llvm/include/llvm/CodeGen/MachineBasicBlock.h
22

Using unsigned sounds good.

The "Values greater than..." sentence is already covered by the "no bytes are emitted" part above, and can be removed.

NickGuy updated this revision to Diff 393138.Dec 9 2021, 6:37 AM
NickGuy marked 2 inline comments as done.
NickGuy updated this revision to Diff 393844.Dec 13 2021, 3:53 AM

Fixes the test to pass with this patch in isolation (i.e. without the child revision on top)

dmgreen added inline comments.Dec 14 2021, 12:25 AM
llvm/lib/CodeGen/MachineBlockPlacement.cpp
32–1

Should the MaxBytes option be used for these alignments too, if we are adding the option?

llvm/test/CodeGen/AArch64/aarch64-p2align-max-bytes.ll
6–7

Can you add a comment explaining what this is testing. It might be a bit awkward to maintain if the codegen changes and a comment explaining what this is testing will help with that.

NickGuy updated this revision to Diff 394513.Dec 15 2021, 3:36 AM
NickGuy marked 2 inline comments as done.
dmgreen accepted this revision.Jan 4 2022, 12:57 AM

Thanks. Together with D114879 this LGTM.

llvm/include/llvm/CodeGen/TargetLowering.h
9

Add a quick comment for the MaxBytesForAlignment?

SjoerdMeijer added inline comments.Jan 4 2022, 1:03 AM
llvm/include/llvm/CodeGen/MachineBasicBlock.h
23

Just adding this nit before you commit this: missing punctuation here and in most other comments.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 5 2022, 4:54 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

This patch is likely responsible for https://lab.llvm.org/buildbot/#/builders/5/builds/16829 and some other bots.

vitalybuka added a comment.EditedJan 5 2022, 11:07 AM

Here is report with origins from https://lab.llvm.org/buildbot/#/builders/74/builds/8671/steps/18/logs/stdio

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 
FAIL: LLVM :: Transforms/LoopVectorize/X86/fp64_to_uint32-cost-model.ll (65880 of 81093)
******************** TEST 'LLVM :: Transforms/LoopVectorize/X86/fp64_to_uint32-cost-model.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build_msan_track_origins/bin/opt < /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/test/Transforms/LoopVectorize/X86/fp64_to_uint32-cost-model.ll -mcpu=core-avx2 -loop-vectorize -S | /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build_msan_track_origins/bin/llc -mcpu=core-avx2 | /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build_msan_track_origins/bin/FileCheck /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/test/Transforms/LoopVectorize/X86/fp64_to_uint32-cost-model.ll
--
Exit Code: 2
Command Output (stderr):
--
==74358==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xbf34382 in (anonymous namespace)::MCAsmStreamer::emitValueToAlignment(unsigned int, long, unsigned int, unsigned int) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/MC/MCAsmStreamer.cpp:1425:11
    #1 0x96f5097 in emitAlignment /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2494:18
    #2 0x96f5097 in llvm::AsmPrinter::emitBasicBlockStart(llvm::MachineBasicBlock const&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3290:5
    #3 0x96c528d in llvm::AsmPrinter::emitFunctionBody() /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1275:5
    #4 0x745fbdb in llvm::X86AsmPrinter::runOnMachineFunction(llvm::MachineFunction&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/Target/X86/X86AsmPrinter.cpp:82:3
    #5 0xa22715a in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:72:13
    #6 0xb9c2518 in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1437:27
    #7 0xb9ec856 in llvm::FPPassManager::runOnModule(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1483:16
    #8 0xb9c49c2 in runOnModule /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1552:27
    #9 0xb9c49c2 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:542:44
    #10 0xb9ed62b in llvm::legacy::PassManager::run(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1679:14
    #11 0x2d964b9 in compileModule(char**, llvm::LLVMContext&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/tools/llc/llc.cpp:719:8
    #12 0x2d8cbd8 in main /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/tools/llc/llc.cpp:417:22
    #13 0x7ff258bad09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) (BuildId: 18b9a9a8c523e5cfe5b5d946d605d09242f09798)
    #14 0x2cebea9 in _start (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build_msan_track_origins/bin/llc+0x2cebea9)

  Uninitialized value was stored to memory at
    #0 0xa06bbc3 in alignBlocks /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/CodeGen/MachineBlockPlacement.cpp
    #1 0xa06bbc3 in (anonymous namespace)::MachineBlockPlacement::runOnMachineFunction(llvm::MachineFunction&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/CodeGen/MachineBlockPlacement.cpp:3434:3
    #2 0xa22715a in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:72:13
    #3 0xb9c2518 in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1437:27
    #4 0xb9ec856 in llvm::FPPassManager::runOnModule(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1483:16
    #5 0xb9c49c2 in runOnModule /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1552:27
    #6 0xb9c49c2 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:542:44
    #7 0xb9ed62b in llvm::legacy::PassManager::run(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1679:14
    #8 0x2d964b9 in compileModule(char**, llvm::LLVMContext&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/tools/llc/llc.cpp:719:8
    #9 0x2d8cbd8 in main /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/tools/llc/llc.cpp:417:22
    #10 0x7ff258bad09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) (BuildId: 18b9a9a8c523e5cfe5b5d946d605d09242f09798)

  Uninitialized value was created by a heap allocation
    #0 0x2d7d613 in operator new(unsigned long) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/compiler-rt/lib/msan/msan_new_delete.cpp:45:35
    #1 0x7e2ba8d in make_unique<llvm::X86Subtarget, const llvm::Triple &, llvm::StringRef &, llvm::StringRef &, llvm::StringRef &, const llvm::X86TargetMachine &, llvm::MaybeAlign, unsigned int &, unsigned int &> /b/sanitizer-x86_64-linux-bootstrap-msan/build/libcxx_build_msan_track_origins/include/c++/v1/__memory/unique_ptr.h:725:28
    #2 0x7e2ba8d in llvm::X86TargetMachine::getSubtargetImpl(llvm::Function const&) const /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/Target/X86/X86TargetMachine.cpp:315:9
    #3 0x9a32a92 in (anonymous namespace)::AtomicExpand::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/CodeGen/AtomicExpandPass.cpp:175:11
    #4 0xb9c2518 in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1437:27
    #5 0xb9ec856 in llvm::FPPassManager::runOnModule(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1483:16
    #6 0xb9c49c2 in runOnModule /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1552:27
    #7 0xb9c49c2 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:542:44
    #8 0xb9ed62b in llvm::legacy::PassManager::run(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1679:14
    #9 0x2d964b9 in compileModule(char**, llvm::LLVMContext&) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/tools/llc/llc.cpp:719:8
    #10 0x2d8cbd8 in main /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/tools/llc/llc.cpp:417:22
    #11 0x7ff258bad09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) (BuildId: 18b9a9a8c523e5cfe5b5d946d605d09242f09798)
SUMMARY: MemorySanitizer: use-of-uninitialized-value /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/MC/MCAsmStreamer.cpp:1425:11 in (anonymous namespace)::MCAsmStreamer::emitValueToAlignment(unsigned int, long, unsigned int, unsigned int)
Exiting
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build_msan_track_origins/bin/FileCheck /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/test/Transforms/LoopVectorize/X86/fp64_to_uint32-cost-model.ll

Thanks for the report. Sounds like the sanitizer is doing its job admirably.

I'll try and make a fix. I think that MaxBytesForAlignment in TLI is the thing that isn't being initialized, but need to test it.