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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
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>
----------------
jyknight wrote:
> 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.
> 
> 
Ah, thanks! (I hadn't looked closely enough to see the semantics of the change, was just generally surprised at any test changes - but totally make sense, thanks for explaining)


================
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,
----------------
jyknight wrote:
> 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.
Ugh. Pity - this file goes back to 2002, so I imagine it came in in the Time Before Testing (not really - but the older stuff tends to be a bit less well tested, especially in side utilities like this)

Ah well. Thanks!


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

https://reviews.llvm.org/D57172