[PATCH] D64108: [GlobalISel][AArch64] Use getConstantVRegValWithLookThrough for selectArithImmed
Quentin Colombet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 2 16:08:44 PDT 2019
qcolombet requested changes to this revision.
qcolombet added a comment.
This revision now requires changes to proceed.
General direction LGTM but there's a bug in the current patch unless I am mistaken.
As a general note, if we need to use this specific helper in more places, I think we should fix the way we handle constant like I already mentioned in https://reviews.llvm.org/D59227.
================
Comment at: llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp:3672
+ return None;
+ MachineInstr *Def = MRI.getVRegDef(ValAndVReg->VReg);
if (Def->getOpcode() != TargetOpcode::G_CONSTANT)
----------------
You can get the value directly with ValAndVReg->Value, instead of checking for the G_CONSTANT and such.
This is actually better that ZExt the value yourself because the constant that you're reading could have been sext or truncated, etc. and you're code misses that.
================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/select-cmp.mir:90
+ %1:gpr(s64) = G_CONSTANT i64 42
+ %2:gpr(s32) = G_TRUNC %1(s64)
+ %5:gpr(s32) = G_ICMP intpred(eq), %0(s32), %2
----------------
Could you write a test that would make the actual 64-bit constant to drop some bits when truncated to 32-bit?
That way that would catch the bug I was mentioning in this patch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64108/new/
https://reviews.llvm.org/D64108
More information about the llvm-commits
mailing list