This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Add type mangling to llvm.amdgcn.readfirstlane
AbandonedPublic

Authored by arsenm on Jul 27 2020, 5:12 AM.

Details

Summary

I've thought this should at minimum work for all the legal 32-bit
types for a long time. Arguably we should also legalize other types,
or at least multiples of 32.

This fixes selection for non-s32 types when readfirstlane is inserted
for SGPR return values.

This will require API users to add the type mangling when getting the
declaration. I think the way mesa constructs intrinsics will happen to
get away without needing to change anything.

Diff Detail

Event Timeline

arsenm created this revision.Jul 27 2020, 5:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2020, 5:12 AM

Also not sure if I should change readlane/writelane at the same time

I think this makes sense, but it is a flag-day change for both Mesa and LLPC. Intrinsics created with incorrect mangling don't fall over completely (as evidenced by the fact that your change *doesn't* change the test files), but certain things like attributes tend to not work quite correctly anymore.

So I think we can still do this change, but please do all of readlane, readfirstlane, and writelane in a single change so that the pain is minimized.

You do have have to fix all test cases to use the correct mangling.

On second thought, can we make this not be a flag-day change?

Also, are you currently working on this? If not, I may be interested in picking it up.

On second thought, can we make this not be a flag-day change?

Also, are you currently working on this? If not, I may be interested in picking it up.

I'm not actively working on this. This only fixes 1 lit test which I switch the default to GlobalISel, so it's not particularly high on my priority list

foad added inline comments.Aug 17 2020, 3:34 AM
llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp
609

Hoist this above the "if" on line 605?

D86154 is an attempt to do essentially this, on all lane intrinsics simultaneously, giving them a new name to avoid a flag day change.

arsenm abandoned this revision.Jun 22 2023, 3:12 AM

Obsoleted by D147732

Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2023, 3:12 AM
Herald added a subscriber: StephenFan. · View Herald Transcript