Page MenuHomePhabricator

[AMDGPU] Enable divergence-driven BFE selection
ClosedPublic

Authored by alex-t on Oct 1 2021, 11:05 AM.

Details

Summary

Detailed description: This change enables the bit field extract patterns
selection to s_bfe_u32 or v_bfe_u32 dependent on the pattern root node
divergence.

Diff Detail

Event Timeline

alex-t created this revision.Oct 1 2021, 11:05 AM
alex-t requested review of this revision.Oct 1 2021, 11:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2021, 11:05 AM
Herald added a subscriber: wdng. · View Herald Transcript
foad added a comment.Oct 1 2021, 12:20 PM

There are also patterns in SIInstructions.td that generate S_BFE without checking that the node is uniform.

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
43–44

How about replacing this with a single getBFE32 function, which takes "bool isSigned" and "bool isDivergent" arguments instead of "unsigned Opcode"?

alex-t updated this revision to Diff 383680.Sun, Oct 31, 1:37 PM

sign_extend_inreg pattterns added
getS_BFE and getV_BFE replaced with the unified getBFE32 function

alex-t marked an inline comment as done.Sun, Oct 31, 1:40 PM
rampitec added inline comments.Mon, Nov 1, 12:10 PM
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1968

Cannot IsDivergent be derived from the Val itself? Like SDNode(Val, 0)->isDivergent()?

llvm/test/CodeGen/AMDGPU/divergence-driven-bfe-isel.ll
5

Check constants used in the instructions too.

arsenm added inline comments.Mon, Nov 1, 1:34 PM
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1968

No else after return

alex-t added inline comments.Tue, Nov 2, 2:32 PM
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1968

I am not sure if this is the right way. The "Val" is N->getOperand(0) here. Formally we should consider the N divergence. On the other hand, operand 0 is the only variable operand in BFE SDNode. Two others (offset and width) are always ConstantSDNode.
Thus, in fact, the divergence property of N is always equal to the divergence of its variable source. From this point of view, we can evolve the N divergence from
N->getOperand(0) divergence. I could save one formal argument, but this would make the code non-obvious and would require some comments to explain.

rampitec added inline comments.Tue, Nov 2, 2:37 PM
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1968

Yes, that's what mean, the rest of arguments are constants, so we can save the argument.

alex-t updated this revision to Diff 384250.Tue, Nov 2, 2:59 PM

test updated to check constants, no else after return.

alex-t updated this revision to Diff 384268.Tue, Nov 2, 4:11 PM

in getBFE32 IsDivergent argument removed. BFE node divergence evolved from its variable operand.

alex-t marked 3 inline comments as done.Tue, Nov 2, 4:13 PM
rampitec accepted this revision.Tue, Nov 2, 4:13 PM

LGTM

This revision is now accepted and ready to land.Tue, Nov 2, 4:13 PM
This revision was landed with ongoing or failed builds.Wed, Nov 3, 1:25 PM
This revision was automatically updated to reflect the committed changes.