This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Add new combine to convert G_MUL to G_SHL.
ClosedPublic

Authored by aemerson on Jan 29 2020, 12:46 PM.

Details

Diff Detail

Event Timeline

aemerson created this revision.Jan 29 2020, 12:46 PM

@arsenm Matt when I run the update_llc_test_checks script on the llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.update.dpp.ll test, it crashes with a backtrace like this:

Assertion failed: (Opcode < NumOpcodes && "Invalid opcode!"), function get, file /Users/aemerson/work/oss/co1/src/llvm/include/llvm/MC/MCInstrInfo.h, line 4
frame #4: 0x00000001000bdef3 llc`llvm::MCInstrInfo::get(this=0x0000000110010248, Opcode=4294967295) const at MCInstrInfo.h:45:5
frame #5: 0x000000010083b8d7 llc`llvm::SIInstrInfo::getMCOpcodeFromPseudo(this=0x0000000110010240, Opcode=3758) const at SIInstrInfo.h:939:12
frame #6: 0x00000001008277e3 llc`llvm::SIInstrInfo::getInstSizeInBytes(this=0x0000000110010240, MI=0x000000010f879b28) const at SIInstrInfo.cpp:6067:29
frame #7: 0x0000000101f9bf44 llc`(anonymous namespace)::BranchRelaxation::computeBlockSize(this=0x000000010f03fb60, MBB=0x000000010f8bbf28) const at BranchRelaxation.cpp:168:18
frame #8: 0x0000000101f9b7be llc`(anonymous namespace)::BranchRelaxation::scanFunction(this=0x000000010f03fb60) at BranchRelaxation.cpp:158:39
frame #9: 0x0000000101f9b264 llc`(anonymous namespace)::BranchRelaxation::runOnMachineFunction(this=0x000000010f03fb60, mf=0x000000010f0566f0) at BranchRelaxation.cpp:555:3

I'm replacing a G_MUL with an equivalent G_SHL here, is there an issue with doing this for AMDGPU?

@arsenm Matt when I run the update_llc_test_checks script on the llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.update.dpp.ll test, it crashes with a backtrace like this:

Assertion failed: (Opcode < NumOpcodes && "Invalid opcode!"), function get, file /Users/aemerson/work/oss/co1/src/llvm/include/llvm/MC/MCInstrInfo.h, line 4
frame #4: 0x00000001000bdef3 llc`llvm::MCInstrInfo::get(this=0x0000000110010248, Opcode=4294967295) const at MCInstrInfo.h:45:5
frame #5: 0x000000010083b8d7 llc`llvm::SIInstrInfo::getMCOpcodeFromPseudo(this=0x0000000110010240, Opcode=3758) const at SIInstrInfo.h:939:12
frame #6: 0x00000001008277e3 llc`llvm::SIInstrInfo::getInstSizeInBytes(this=0x0000000110010240, MI=0x000000010f879b28) const at SIInstrInfo.cpp:6067:29
frame #7: 0x0000000101f9bf44 llc`(anonymous namespace)::BranchRelaxation::computeBlockSize(this=0x000000010f03fb60, MBB=0x000000010f8bbf28) const at BranchRelaxation.cpp:168:18
frame #8: 0x0000000101f9b7be llc`(anonymous namespace)::BranchRelaxation::scanFunction(this=0x000000010f03fb60) at BranchRelaxation.cpp:158:39
frame #9: 0x0000000101f9b264 llc`(anonymous namespace)::BranchRelaxation::runOnMachineFunction(this=0x000000010f03fb60, mf=0x000000010f0566f0) at BranchRelaxation.cpp:555:3

I'm replacing a G_MUL with an equivalent G_SHL here, is there an issue with doing this for AMDGPU?

No, this is probably one of the more important optimizations to do. I think the problem you're hitting here is there's some confusion in the selection predicates for sub targets which removed the non-reversed operand forms. I think D73483 should fix that when committed

llvm/include/llvm/Target/GlobalISel/Combine.td
138

lsl->shl to match the instruction name?

arsenm added inline comments.Jan 29 2020, 1:10 PM
llvm/test/CodeGen/AArch64/GlobalISel/combine-mul-to-lsl.mir
72 ↗(On Diff #241260)

Add some vector cases to make sure they at least don't crash?

aemerson updated this revision to Diff 241274.Jan 29 2020, 1:27 PM

I've made the test XFAIL for now. Address other comments.

aemerson updated this revision to Diff 241275.Jan 29 2020, 1:28 PM

Forgot to rename test file.

arsenm accepted this revision.Jan 29 2020, 1:37 PM
This revision is now accepted and ready to land.Jan 29 2020, 1:37 PM
This revision was automatically updated to reflect the committed changes.