This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Fold more inline constant operands by commuting instructions
AbandonedPublic

Authored by foad on Nov 19 2021, 3:22 AM.

Details

Reviewers
arsenm
rampitec
Summary

SIFoldOperands::foldInstOperand folds an immediate value into any number
of inline immediates uses, but it was missing uses that could be inline
if the instruction was commuted. The test diffs mostly show cases where
the immediate value 0 can be folded into more than one commuted v_addc
instruction.

Unfortunately this adds more duplication between
isInlineConstantIfFolded and tryAddToFoldList, but there was already
duplication of the mac/mad handling.

Diff Detail

Event Timeline

foad created this revision.Nov 19 2021, 3:22 AM
foad requested review of this revision.Nov 19 2021, 3:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2021, 3:22 AM

A dedicated test (maybe MIR) would be nice

foad updated this revision to Diff 388498.Nov 19 2021, 7:26 AM

Add a MIR test.

foad added inline comments.Nov 19 2021, 7:29 AM
llvm/test/CodeGen/AMDGPU/fold-multiple-commute.mir
18

It occurs to me now that SIFoldOperands would probably have succeeded here if these were using the e64 form of the instruction. But there are obviously real world cases where it sees the e32 form, otherwise none of the .ll tests would have been improved by this patch.

Is it worth abandoning this patch and pursuing why we are selecting e32 instructions in the first place?

arsenm added inline comments.Nov 23 2021, 3:02 PM
llvm/test/CodeGen/AMDGPU/fold-multiple-commute.mir
18

I don't know about abandoning, but it should be looked into. We're mostly consistent in picking the e64 forms upfront

foad abandoned this revision.Sep 7 2022, 3:19 AM

This is obsoleted by D114643 which removed isInlineConstantIfFolded. I committed the new test case from this patch in 01c53d7d80eada4db2c63df28bfbeeb0e2f8cad7 to verify that this is no longer required.

Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 3:19 AM
Herald added a subscriber: kosarev. · View Herald Transcript