This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Remove permlane discard vdst_in optimization from isel
ClosedPublic

Authored by foad on Dec 22 2022, 4:04 AM.

Details

Summary

D72845 implemented the equivalent IR optimization in InstCombine so it
seems that there's no advantage to doing it during isel too.

This partially reverts D72844.

Diff Detail

Event Timeline

foad created this revision.Dec 22 2022, 4:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2022, 4:04 AM
foad requested review of this revision.Dec 22 2022, 4:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2022, 4:04 AM
arsenm accepted this revision.Dec 22 2022, 6:05 AM

LGTM with test nit

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.permlane.ll
853

Should use poison

1113

Can you also change these "i1 1" and "i1 0" to "i1 true" and "i1 false" while you're touching the lines

This revision is now accepted and ready to land.Dec 22 2022, 6:05 AM
foad added inline comments.Dec 22 2022, 6:39 AM
llvm/test/CodeGen/AMDGPU/llvm.amdgcn.permlane.ll
853

Really? I read that "Most instructions return ‘poison’ when one of their arguments is ‘poison’" and that's not what I want here. Besides I am just mimicking InstCombine which still uses undef for this optimization.

arsenm added inline comments.Dec 22 2022, 6:41 AM
llvm/test/CodeGen/AMDGPU/llvm.amdgcn.permlane.ll
853

undef is soft deprecated. Undef should be freeze poison

foad added a subscriber: nlopes.Dec 22 2022, 6:47 AM
foad added inline comments.
llvm/test/CodeGen/AMDGPU/llvm.amdgcn.permlane.ll
853

+ @nlopes So is "freeze poison" the best option here? I guess just "poison" would work, since nothing is actually going to remove the whole intrinsic in a codegen test, but it seems conceptually wrong.

foad updated this revision to Diff 484823.Dec 22 2022, 6:48 AM

Use true and false.

arsenm accepted this revision.Dec 22 2022, 6:56 AM
nlopes added inline comments.Dec 22 2022, 6:59 AM
llvm/test/CodeGen/AMDGPU/llvm.amdgcn.permlane.ll
853

llvm.amdgcn.permlane16 doesn't seem to be documented, so I have no clue what it does.
If poison is not a good value here, and a constant is not what you want, you can always add a new i32 argument to the test function and use it. That's how you get a fresh variable for free.

Thanks for not using undef btw!

foad updated this revision to Diff 484829.Dec 22 2022, 7:09 AM

Use freeze poison.

arsenm accepted this revision.Dec 22 2022, 7:11 AM