[PATCH] D57172: [opaque pointer types] Pass value type to LoadInst creation.

[James Y Knight via Phabricator via llvm-commits]

jyknight marked 2 inline comments as done.
jyknight added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/lower-kernargs.ll:309-311
+; HSA-NEXT:    [[ARG0_KERNARG_OFFSET_CAST:%.*]] = bitcast i8 addrspace(4)* [[ARG0_KERNARG_OFFSET]] to <4 x i32> addrspace(4)*
+; HSA-NEXT:    [[TMP:%.*]] = load <4 x i32>, <4 x i32> addrspace(4)* [[ARG0_KERNARG_OFFSET_CAST]], align 16, !invariant.load !0
+; HSA-NEXT:    [[ARG0_LOAD:%.*]] = shufflevector <4 x i32> [[TMP]], <4 x i32> undef, <3 x i32> <i32 0, i32 1, i32 2>
----------------
dblaikie wrote:
> I haven't figured out why these tests changed - could you explain it?
This is due to the change in lvm/lib/Target/AMDGPU/AMDGPULowerKernelArguments.cpp

This diff hilighting here is confusing, but all that's actually changed is that an extraneous bitcast was removed.

It used to go:
ARG0_KERNARG_OFFSET_CAST = bitcast ARG0_KERNARG_OFFSET to <3 x i32>*
TMP1 = bitcast ARG0_KERNARG_OFFSET_CAST to <4 x i32>*
TMP2 = load TMP1
ARG0_LOAD = shufflevector TMP2

In the new version, all that's different is that the first bitcast is deleted, and the lit match-names are different.




================
Comment at: llvm/tools/bugpoint/Miscompilation.cpp:884
+          Value *CachedVal =
+              new LoadInst(F->getFunctionType(), Cache, "fpcache", EntryBB);
           Value *IsNull = new ICmpInst(*EntryBB, ICmpInst::ICMP_EQ, CachedVal,
----------------
dblaikie wrote:
> This doesn't immediately make sense to me - can you load a function type? What's the resulting value type? (I guess I'm misreading this somehow)
No, this change is wrong, it should've been F->getType(), to match line 867. I guess this codepath is untested, because if it was run, it'd assert fail in the LoadInst constructor.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57172/new/

https://reviews.llvm.org/D57172