This is an archive of the discontinued LLVM Phabricator instance.

[opaque pointer types] Pass value type to LoadInst creation.
ClosedPublic

Authored by jyknight on Jan 24 2019, 10:54 AM.

Details

Summary

This cleans up all LoadInst creation in LLVM to explicitly pass the
value type rather than deriving it from the pointer's element-type.

Diff Detail

Repository
rL LLVM

Event Timeline

jyknight created this revision.Jan 24 2019, 10:54 AM
dblaikie added inline comments.Jan 24 2019, 1:44 PM
llvm/test/CodeGen/AMDGPU/lower-kernargs.ll
309–311 ↗(On Diff #183344)

I haven't figured out why these tests changed - could you explain it?

llvm/tools/bugpoint/Miscompilation.cpp
884 ↗(On Diff #183344)

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)

jyknight marked 2 inline comments as done.Jan 27 2019, 3:58 PM
jyknight added inline comments.
llvm/test/CodeGen/AMDGPU/lower-kernargs.ll
309–311 ↗(On Diff #183344)

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.

llvm/tools/bugpoint/Miscompilation.cpp
884 ↗(On Diff #183344)

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.

jyknight updated this revision to Diff 183798.Jan 27 2019, 8:50 PM

Rebase and fix one commented issue.

dblaikie accepted this revision.Jan 28 2019, 2:04 PM
dblaikie added inline comments.
llvm/test/CodeGen/AMDGPU/lower-kernargs.ll
309–311 ↗(On Diff #183344)

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)

llvm/tools/bugpoint/Miscompilation.cpp
884 ↗(On Diff #183344)

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!

This revision is now accepted and ready to land.Jan 28 2019, 2:04 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 12:44 PM