[PATCH] D64108: [GlobalISel][AArch64] Use getConstantVRegValWithLookThrough for selectArithImmed

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