This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISel: Adjust image load register type based on dmask
ClosedPublic

Authored by arsenm on Jan 29 2020, 2:30 PM.

Details

Reviewers
nhaehnle
kerbowa
Summary

Trim elements that won't be written. The equivalent still needs to be
done for writes. Also start widening 3 elements to 4
elements. Selection will get the count from the dmask.

Diff Detail

Event Timeline

arsenm created this revision.Jan 29 2020, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2020, 2:30 PM
arsenm updated this revision to Diff 242889.Feb 6 2020, 6:09 AM

Generate new test checks

Mostly looks good, but I do have some questions.

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
3201

Why the type change? DMaskLanes doesn't need to be signed, does it?

3210

This seems like it might mess with atomics.

RMW atomics are 32- or 64-bits depending on whether dmask is 1 or 3. cmpswap atomics are 32- or 64-bits depending on whether dmask is 3 or 15.

I guess you're not testing atomics on the GlobalISel path yet?

3254

You do now have an Observer argument here, right?

arsenm marked 2 inline comments as done.Feb 7 2020, 5:51 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
3201

The parts below are using int, and it was less effort to swsitxch everything to int (which I guess is what the current guidance is anyway)

3210

I'm ignoring atomics for now until I have selection working for the normal load/stores

nhaehnle accepted this revision.Mar 17 2020, 3:52 AM

Minor comments inline, LGTM apart from that.

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
3254

I'm assuming this will be addressed by the scoped observer exit introduced with the previous commit. Please remember to remove the comment.

3268

As above: please remember to remove the comment.

This revision is now accepted and ready to land.Mar 17 2020, 3:52 AM