This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Select G_SEXT/G_ZEXT/G_ANYEXT
ClosedPublic

Authored by arsenm on Jun 24 2019, 8:19 PM.

Details

Reviewers
tstellar
nhaehnle

Diff Detail

Event Timeline

arsenm created this revision.Jun 24 2019, 8:19 PM
nhaehnle accepted this revision.Jun 25 2019, 5:12 AM

Trivial nitpick, but essentially LGTM.

lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
386–387

clang-format?

690–696

Out of curiosity: The unsigned case of this can be implement using v_and_b32 as well, which would be more compact for small source sizes if s4 or similar was legal, since it can be encoded as VOP2.

This is a micro-optimization that I believe doesn't even apply right now, but in general where is that kind of thing expected to be handled ultimately? Here in the instruction selector, or elsewhere in a separate machine instruction combining pass?

lib/Target/AMDGPU/AMDGPUInstructionSelector.h
67

selectG_EXT perhaps?

This revision is now accepted and ready to land.Jun 25 2019, 5:12 AM
arsenm marked an inline comment as done.Jun 25 2019, 5:44 AM
arsenm added inline comments.
lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
690–696

It would only really be smaller if the mask value is an inline immediate. For this and the scalar 64-bit case, you could make a code size tradeoff by materializing the constant for multiple uses.

We do similar things in SIShrinkInstructions, but I think most of the cases that appear there are due to the lack of context in SelectionDAG. I would probably want to try to handle it up front here.

arsenm closed this revision.Jun 25 2019, 6:18 AM
arsenm marked an inline comment as done.

r364308

lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
386–387

I took this part, but in general I think clang-format mangles BuildMI formatting