[PATCH] D50931: Consider isLegalAddressingImm in Constant Hoisting

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:594
+          break;
+        } else if (StoreInst *SI = dyn_cast<StoreInst>(UI)) {
+          MemUseValTy = SI->getValueOperand()->getType();
----------------
Oh, oops, should have spotted the issue here sooner.

This is checking that the use is a store instruction, but it isn't checking that the value is being used as an address.  So it'll do the wrong thing for an instruction like `store i32* inttoptr (i32 805874688 to i16*), i32** %foo`.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:608
+           /*BaseGV*/nullptr, /*BaseOffset*/Diff.getSExtValue(),
+           /*HasBaseReg*/false, /*Scale*/0)))
         continue;
----------------
I think HasBaseReg should be true; the base is the hoisted constant.  Not sure that actually makes a difference in practice; the ARM backend, at least, doesn't check HasBaseReg when the Scale is zero.


Repository:
  rL LLVM

https://reviews.llvm.org/D50931