This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Treat texture gather instructions more like other MIMG instructions
ClosedPublic

Authored by nhaehnle on Jul 11 2016, 3:45 AM.

Details

Summary

Setting MIMG to 0 has a bunch of unexpected side effects, including that
isVMEM returns false which leads to incorrect treatment in the hazard
recognizer. The reason I noticed it is that it also leads to incorrect
treatment in VGPR-to-SGPR copies, which is one cause of the referenced bug.

The only reason why MIMG was set to 0 is to signal the special handling of
dmasks, but that can be checked differently.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96877

Diff Detail

Event Timeline

nhaehnle updated this revision to Diff 63483.Jul 11 2016, 3:45 AM
nhaehnle retitled this revision from to AMDGPU: Treat texture gather instructions more like other MIMG instructions.
nhaehnle updated this object.
nhaehnle added reviewers: arsenm, tstellarAMD.
nhaehnle added a subscriber: llvm-commits.
arsenm added inline comments.Jul 11 2016, 10:03 AM
lib/Target/AMDGPU/SIISelLowering.cpp
3136–3137

I don't understand why the check for hasPostISelHook is here. It shouldn't be a relevant property to check here

test/CodeGen/AMDGPU/llvm.SI.gather4.ll
468–470

Needs some check lines

nhaehnle added inline comments.Jul 11 2016, 12:00 PM
lib/Target/AMDGPU/SIISelLowering.cpp
3136–3137

What we really want to check is whether the MIMG instructions' dmask has the default treatment (i.e. bitfield of returned components) as opposed to the special gather4 treatment.

For MIMG instructions, this coincides precisely with the hasPostISelHook, because fixing up the dmask is precisely what the PostISelHook is used for.

I could add an entry to the TSFlags for this bit, though I didn't do it because the information is redundant. Would you prefer the TSFlags solution?

nhaehnle updated this revision to Diff 63551.Jul 11 2016, 12:09 PM

Add some CHECK lines to the new test.

nhaehnle marked an inline comment as done.Jul 11 2016, 12:09 PM
arsenm added inline comments.Jul 11 2016, 12:32 PM
lib/Target/AMDGPU/SIISelLowering.cpp
3136–3138

Is dmask an operand? can you just see if getNamedOperandIdx says it has the operand?

arsenm added inline comments.Jul 11 2016, 12:35 PM
lib/Target/AMDGPU/SIISelLowering.cpp
3136–3138

But yes, a new flag would be better than repurposing this one

nhaehnle updated this revision to Diff 63575.Jul 11 2016, 2:29 PM
nhaehnle marked 2 inline comments as done.

The trouble is that the operand is named "dmask" for all image sample _and_
gather4 instructions :)

Added the Gather4 flag.

arsenm added inline comments.Jul 11 2016, 2:46 PM
lib/Target/AMDGPU/SIISelLowering.cpp
3137

This should be a new check in SIInstrInfo. Would mayLoad be better than !mayStore?

nhaehnle updated this revision to Diff 63588.Jul 11 2016, 2:57 PM

Add isGather4 in SIInstrInfo.

!mayStore is the correct check, because we need to account for image
atomics here.

arsenm accepted this revision.Jul 11 2016, 3:03 PM
arsenm edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jul 11 2016, 3:03 PM
This revision was automatically updated to reflect the committed changes.