This is an archive of the discontinued LLVM Phabricator instance.

[SDAG] narrow truncated sign_extend_inreg
ClosedPublic

Authored by spatel on Jul 15 2022, 11:56 AM.

Details

Summary

trunc (sign_ext_inreg X, iM) to iN --> sign_ext_inreg (trunc X to iN), iM

There are improvements on existing tests from this, and there are a pair of large regressions in D127115 for Thumb2 caused by not folding this pattern.

Diff Detail

Event Timeline

spatel created this revision.Jul 15 2022, 11:56 AM
spatel requested review of this revision.Jul 15 2022, 11:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2022, 11:56 AM
spatel updated this revision to Diff 445231.Jul 16 2022, 6:31 AM
spatel retitled this revision from [SDAG] reduce cast ops around sign_extend_inreg to [SDAG] narrow truncated sign_extend_inreg.
spatel edited the summary of this revision. (Show Details)
spatel added reviewers: foad, arsenm.

Patch updated:
On closer inspection, we can generalize the fold to only match trunc-of-sext (don't need to match the preceding extend), and that shows improvements on existing tests for x86 and (I think) AMDGPU.

arsenm accepted this revision.Jul 16 2022, 6:33 AM
arsenm added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13138

Shouldn't really require legal types, but the DAG is lacking in adequate legality checks for sext_inreg

llvm/test/CodeGen/AMDGPU/mul_int24.ll
184–187

This is a pretty big improvement

This revision is now accepted and ready to land.Jul 16 2022, 6:33 AM
spatel marked 2 inline comments as done.Jul 16 2022, 6:46 AM
spatel added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13138

There are a couple of other sext_inreg folds in DAGCombiner that use something like this:

if (!LegalOperations ||
    TLI.getOperationAction(ISD::SIGN_EXTEND_INREG, ExtVT) ==
    TargetLowering::Legal)

I tried that and don't see any differences vs. this check, so I could use that too, but I think the current predicate would be more conservative just in case there's some unexpected type conversions during legalization.

llvm/test/CodeGen/AMDGPU/mul_int24.ll
184–187

Thanks for confirming!

arsenm added inline comments.Jul 16 2022, 7:09 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13138

This is one of the places where we really need 2 type checks, but I think checking the ExtVT legality is probably enough here

This revision was landed with ongoing or failed builds.Jul 16 2022, 1:29 PM
This revision was automatically updated to reflect the committed changes.
spatel marked 2 inline comments as done.