This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Handle multiple uses when matching sincos
ClosedPublic

Authored by arsenm on Jul 31 2023, 8:55 AM.

Details

Reviewers
rampitec
Pierre-vh
vpykhtin
dfukalov
jmmartinez
yaxunl
jhuber6
Group Reviewers
Restricted Project
Summary

Match how the generic implementation handles this. We now will leave
behind the dead other user for later passes to deal with.

Diff Detail

Event Timeline

arsenm created this revision.Jul 31 2023, 8:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2023, 8:55 AM
arsenm requested review of this revision.Jul 31 2023, 8:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2023, 8:55 AM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm updated this revision to Diff 546443.Aug 2 2023, 6:39 AM

rebase, doesn't bother trying to refine cos load debug loc

jmmartinez added inline comments.Aug 4 2023, 3:11 AM
llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp
1289

Changed is always false, right?

arsenm updated this revision to Diff 547430.Aug 4 2023, 5:58 PM

Remove leftover

Do you think it's worth running a pass like InstSimplify after the tests to check the instruction does get eliminated?
Maybe with a different CHECK line?

Otherwise I think it's error prone, if someone makes a change that makes the inst not dead anymore all we would see in the test diff is an operand that's renamed

llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp
94–99

nit: add docs, or use a small struct, otherwise it's difficult to tell what the tuple contains

1251–1262

small nit: avoids repetition

arsenm added a comment.Aug 7 2023, 5:35 AM

Do you think it's worth running a pass like InstSimplify after the tests to check the instruction does get eliminated?

No, it wouldn't be testing anything meaningful. If you don't have the right declaration attributes, it won't be deleted. You'd have to depend on the manually specified attributes in the test. The declarations in whatever source would have to be correct, or the attributes would have to be correct after linking (which is what actually happens)

arsenm updated this revision to Diff 547748.Aug 7 2023, 5:49 AM
jmmartinez accepted this revision.Aug 14 2023, 12:15 AM
This revision is now accepted and ready to land.Aug 14 2023, 12:15 AM