This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Correct memory size for image intrinsics
ClosedPublic

Authored by arsenm on Jan 28 2020, 8:03 AM.

Details

Summary

This was incorrectly rounding up to the next power of 2. v4f32 was
rounding up to v8f32, which was just wrong. There are also v3i16/v3f16
available in MVT, so we don't even need to round the f16 cases
anymore. Additionally, this field is really an EVT so we don't even
need to consider this.

Also switch some asserts to return invalid. We should have an IR
verifier for these intrinsic return types, but for now it's better to
not assert on IR that passes the verifier.

This should also probably be fixed to consider dmask is really
eliminating some of the loaded components.

Diff Detail

Event Timeline

arsenm created this revision.Jan 28 2020, 8:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2020, 8:03 AM

Added David Stuttard as reviewer since this was originally his.

nhaehnle added inline comments.Jan 30 2020, 1:47 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
922–925

I think I'd prefer to have all of this logic inside of memVTFromImageReturn. That would make the function name fit better, since with this change it's really more "memVTFromImageReturnIfItsAStruct"...

arsenm updated this revision to Diff 241432.Jan 30 2020, 6:17 AM

Move to function

nhaehnle accepted this revision.Feb 2 2020, 10:57 AM

Thank you. Memory sizes on image instructions are kind of weird in the first place, because we simply cannot know the actual *memory* size at compile time, but this is a pragmatic thing to do for GlobalISel at the moment. LGTM.

This revision is now accepted and ready to land.Feb 2 2020, 10:57 AM