This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add special case to selectImm for constants that can be created with (ADD (SLLI C, 32), C).
ClosedPublic

Authored by craig.topper on Jun 5 2023, 11:51 PM.

Details

Summary

Where C is a simm32.

This costs an extra temporary register, but avoids a constant pool.

Diff Detail

Event Timeline

craig.topper created this revision.Jun 5 2023, 11:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2023, 11:51 PM
craig.topper requested review of this revision.Jun 5 2023, 11:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2023, 11:51 PM
asb added a comment.Jun 6 2023, 12:59 AM

I'd not really explored such materialisations before as adding another dimension to cost modeling (register usage) felt difficult to reason about...but intuitively I can't see spending an extra register being problematic in this specific case where a constant pool would have been used, and given how relatively register rich RISC-V is.

I am however seeing some compile failures for this e.g. 20000523-1.c from the GCC torture suite (e.g. compiled rv64imafdc for lp64 ABI at O0)

/home/asb/llvm-project/llvm/lib/MC/MCInst.cpp:58: bool llvm::MCOperand::isBareSymbolRef() const: Assertion `isExpr() && "isBareSymbolRef expects only expressions"' failed.
...
reames added a comment.Jun 6 2023, 8:34 AM

General approach seems entirely reasonable to me.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
212

Very minor: This code can produce a sequence with minimum length of 3, so you can avoid running it when the original sequence length is 3.

225

If you swap these operands, does that help add immediate formation when the two halves are very small?

I'm thinking of the case 0x00020002. This can be done as LI + SHL + ADDI. With only a single register.

This might not matter, and we might need a second special case for this, if so, feel free to punt to future patch.

Thinking about, it's probably a subcase of having a general 32 bit constant hi, + a 12 bit low half which can always be done in 4 instructions, and 3 depending on the exact details of the high half.

craig.topper added inline comments.Jun 6 2023, 9:53 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
225

This should be ADD not ADDI.

craig.topper added inline comments.Jun 6 2023, 9:59 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
209

This should be ADD

225

I assume you meant 0x0000000200000002? That should be handled by the base algorithm. https://godbolt.org/z/1rPsbh5a7

The base algorithm is basically subtract sign extended version of lo12 bits, count trailing zeros, shift right by that amount, recurse. The subtract becomes an ADDI and the shift right becomes a SLLI.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4403

This should be ADD

Use ADD instead of ADDI.

reames accepted this revision.Jun 6 2023, 11:44 AM

LGTM

This revision is now accepted and ready to land.Jun 6 2023, 11:44 AM
This revision was landed with ongoing or failed builds.Jun 6 2023, 11:59 AM
This revision was automatically updated to reflect the committed changes.
fmayer added a comment.Jun 6 2023, 5:29 PM
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:214:43: runtime error: signed integer overflow: 9223372034904144827 - -1950630981 cannot be represented in type 'int64_t' (aka 'long')
    #0 0xaaaae4525208 in selectImm(llvm::SelectionDAG*, llvm::SDLoc const&, llvm::MVT, long, llvm::RISCVSubtarget const&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:214:43
    #1 0xaaaae451fff8 in llvm::RISCVDAGToDAGISel::Select(llvm::SDNode*) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:863:23
    #2 0xaaaae6204148 in llvm::SelectionDAGISel::DoInstructionSelection() /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1178:7
    #3 0xaaaae6202418 in llvm::SelectionDAGISel::CodeGenAndEmitDAG() /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:949:5
    #4 0xaaaae61feb54 in llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1700:7
    #5 0xaaaae61fa77c in llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:482:3
    #6 0xaaaae5469750 in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:91:13
    #7 0xaaaae5b2acf0 in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1435:27
    #8 0xaaaae5b34fd0 in llvm::FPPassManager::runOnModule(llvm::Module&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1481:16
    #9 0xaaaae5b2b7f0 in runOnModule /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1550:27
    #10 0xaaaae5b2b7f0 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:535:44
    #11 0xaaaae31ae314 in compileModule(char**, llvm::LLVMContext&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llc/llc.cpp:756:8
    #12 0xaaaae31ac0b8 in main /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llc/llc.cpp:420:22
    #13 0xffffbb03777c  (/lib/aarch64-linux-gnu/libc.so.6+0x2777c) (BuildId: fe1fc959438108d405a33383d7c3f00762a5bb93)
    #14 0xffffbb037854 in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x27854) (BuildId: fe1fc959438108d405a33383d7c3f00762a5bb93)
    #15 0xaaaae3180fec in _start (/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llc+0x5e90fec)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:214:43 in
fmayer reopened this revision.Jun 6 2023, 5:30 PM
This revision is now accepted and ready to land.Jun 6 2023, 5:30 PM
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:214:43: runtime error: signed integer overflow: 9223372034904144827 - -1950630981 cannot be represented in type 'int64_t' (aka 'long')
    #0 0xaaaae4525208 in selectImm(llvm::SelectionDAG*, llvm::SDLoc const&, llvm::MVT, long, llvm::RISCVSubtarget const&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:214:43
    #1 0xaaaae451fff8 in llvm::RISCVDAGToDAGISel::Select(llvm::SDNode*) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:863:23
    #2 0xaaaae6204148 in llvm::SelectionDAGISel::DoInstructionSelection() /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1178:7
    #3 0xaaaae6202418 in llvm::SelectionDAGISel::CodeGenAndEmitDAG() /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:949:5
    #4 0xaaaae61feb54 in llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1700:7
    #5 0xaaaae61fa77c in llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:482:3
    #6 0xaaaae5469750 in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:91:13
    #7 0xaaaae5b2acf0 in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1435:27
    #8 0xaaaae5b34fd0 in llvm::FPPassManager::runOnModule(llvm::Module&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1481:16
    #9 0xaaaae5b2b7f0 in runOnModule /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1550:27
    #10 0xaaaae5b2b7f0 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:535:44
    #11 0xaaaae31ae314 in compileModule(char**, llvm::LLVMContext&) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llc/llc.cpp:756:8
    #12 0xaaaae31ac0b8 in main /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llc/llc.cpp:420:22
    #13 0xffffbb03777c  (/lib/aarch64-linux-gnu/libc.so.6+0x2777c) (BuildId: fe1fc959438108d405a33383d7c3f00762a5bb93)
    #14 0xffffbb037854 in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x27854) (BuildId: fe1fc959438108d405a33383d7c3f00762a5bb93)
    #15 0xaaaae3180fec in _start (/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llc+0x5e90fec)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:214:43 in

Thanks, I'll take a look.

The revert needs a cleanup of a test that was added after this patch was commit. I'll take care of that too.

fmayer added a comment.Jun 6 2023, 5:39 PM

Seems like I made matters worse. Un-reverting this. Please fix the UBSAan error.

craig.topper closed this revision.Jun 6 2023, 9:36 PM

This was recommitted

dtcxzyw added a subscriber: dtcxzyw.Jun 8 2023, 1:11 AM

Our CI reported that this patch caused code bloat (average ~0.1%) in llvm-test-suite compiled with -Os. We can turn this off when optimizing for size (-Os/-Oz).

Our CI reported that this patch caused code bloat (average ~0.1%) in llvm-test-suite compiled with -Os. We can turn this off when optimizing for size (-Os/-Oz).

https://reviews.llvm.org/D152602