This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Tweak lowering of G_SMULO/G_UMULO
ClosedPublic

Authored by foad on Jan 16 2020, 6:49 AM.

Details

Summary

Applying this cleanup:

  • MIRBuilder.buildInstr(TargetOpcode::G_ASHR)
  • .addDef(Shifted)
  • .addUse(Res)
  • .addUse(ShiftAmt); + MIRBuilder.buildAShr(Shifted, Res, ShiftAmt);

caused an assertion failure here:

llc: /home/jayfoad2/git/llvm-project/llvm/lib/CodeGen/MachineRegisterInfo.cpp:404: llvm::MachineInstr *llvm::MachineRegisterInfo::getVRegDef(unsigned int) const: Assertion `(I.atEnd() || std::next(I) == def_instr_end()) && "getVRegDef assumes a single definition or no definition"' failed.

#4  0x00000000050a6d96 in llvm::MachineRegisterInfo::getVRegDef (this=0x74606a0, Reg=2147483650) at /home/jayfoad2/git/llvm-project/llvm/lib/CodeGen/MachineRegisterInfo.cpp:403
#5  0x00000000066148f6 in llvm::getConstantVRegValWithLookThrough (VReg=2147483650, MRI=..., LookThroughInstrs=false, HandleFConstant=true) at /home/jayfoad2/git/llvm-project/llvm/lib/CodeGen/GlobalISel/Utils.cpp:244
#6  0x00000000066147da in llvm::getConstantVRegVal (VReg=2147483650, MRI=...) at /home/jayfoad2/git/llvm-project/llvm/lib/CodeGen/GlobalISel/Utils.cpp:210
#7  0x0000000006615367 in llvm::ConstantFoldBinOp (Opcode=101, Op1=2147483650, Op2=2147483656, MRI=...) at /home/jayfoad2/git/llvm-project/llvm/lib/CodeGen/GlobalISel/Utils.cpp:341
#8  0x000000000657eee0 in llvm::CSEMIRBuilder::buildInstr (this=0x7465010, Opc=101, DstOps=..., SrcOps=..., Flag=...) at /home/jayfoad2/git/llvm-project/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp:160
#9  0x0000000003645958 in llvm::MachineIRBuilder::buildAShr (this=0x7465010, Dst=..., Src0=..., Src1=..., Flags=...) at /home/jayfoad2/git/llvm-project/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:1298
#10 0x00000000065c35b1 in llvm::LegalizerHelper::lower (this=0x7fffffffb5f8, MI=..., TypeIdx=0, Ty=...) at /home/jayfoad2/git/llvm-project/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:2020

because at this point there are two instructions defining Res: the
original G_SMULO/G_UMULO and the new G_MUL that we built. The fix is
to modify the original mul in place, so that there is only ever one
definition of Res.

Diff Detail

Event Timeline

foad created this revision.Jan 16 2020, 6:49 AM

Unit tests: pass. 61909 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

foad updated this revision to Diff 238488.Jan 16 2020, 7:06 AM

Use MIRBuilder.getTII().

Unit tests: pass. 61909 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

aemerson added inline comments.Jan 30 2020, 8:24 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2012

We can go a bit further here and just do:

auto HiPart = MIRBuilder.buildInstr(Opcode, {Ty}, {LHS, RHS});
2023

Same cleanup here:

auto ShiftAmt = MIRBuilder.buildConstant(Ty, ShiftAmt);
auto Shifted = MIRBuilder.buildAshr(Ty, Res, ShiftAmt);
foad updated this revision to Diff 241670.Jan 31 2020, 3:07 AM

Further cleanups as suggested in review.

foad marked 2 inline comments as done.Jan 31 2020, 3:07 AM

Unit tests: pass. 62357 tests passed, 0 failed and 839 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

foad updated this revision to Diff 241674.Jan 31 2020, 3:36 AM

Add const.

Unit tests: pass. 62357 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This revision is now accepted and ready to land.Jan 31 2020, 10:00 AM
aemerson requested changes to this revision.Jan 31 2020, 10:05 AM
This comment was removed by aemerson.
This revision now requires changes to proceed.Jan 31 2020, 10:05 AM
aemerson accepted this revision.Jan 31 2020, 10:06 AM

Didn't mean to reject.

This revision is now accepted and ready to land.Jan 31 2020, 10:06 AM
This revision was automatically updated to reflect the committed changes.