Page MenuHomePhabricator

[DAGCombine] Combine signext_inreg of extract-extend
ClosedPublic

Authored by peterwaller-arm on Aug 9 2022, 8:49 AM.

Details

Summary

The outer signext_inreg is redundant in the following:

Fold (signext_inreg (extract_subvector (zext|anyext|sext iN_value to _) _) from iN)
     -> (extract_subvector (signext iN_value to iM))

Tests are precommitted and clone those by analogy from the AND case in
the same file. Add a negative test to check extension width is handled
correctly.

This patch supersedes D130700.

Diff Detail

Event Timeline

peterwaller-arm created this revision.Aug 9 2022, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 8:49 AM
peterwaller-arm requested review of this revision.Aug 9 2022, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 8:49 AM
RKSimon added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13226

(style) Create this node inside the if() below - otherwise you end up with dead nodes that could possibly affect later transforms

Thanks for the review comments, Simon!

  • Avoid creating dead nodes.
peterwaller-arm marked an inline comment as done.Aug 9 2022, 9:33 AM
RKSimon added inline comments.Aug 10 2022, 3:51 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13228

Does this need an operation legality check for ISD::SIGN_EXTEND ? (!LegalOperations || TLI.isOperationLegal(ISD::SIGN_EXTEND, InnerExtVT))?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13228

The combine only replaces an extant any-extend with an equivalent sign-extend. Is it an unreasonable assumption that if a target supports an any-extend it won't support the equivalent sign or zero extend? I can add the condition if you think this is suspect.

The patch looks good to me @peterwaller-arm! Before I accept I'm just waiting a bit longer about the legality question.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13228

I guess it's not just replacing an ANY_EXTEND with SIGN_EXTEND - it also replaces ZERO_EXTEND with SIGN_EXTEND too, although the transformation looks valid. I guess I have the same question as @peterwaller-arm - would a target have a legal SIGN_EXTEND, but illegal ZERO_ or ANY_EXTEND?

RKSimon added inline comments.Aug 11 2022, 3:47 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13228

Its unlikely I agree - are the tests affected if we add the legality check?

peterwaller-arm marked 3 inline comments as done.

Ihanks @RKSimon. The tests are unaffected. I've opted to follow your suggestion.

  • Add legality check.
  • Also add legality check for analagous zero-extending case in visitAND cc @david-arm.
  • Fix copy-paste error for the legality check for the ZERO_EXTEND case.
This revision is now accepted and ready to land.Aug 11 2022, 5:57 AM
RKSimon accepted this revision.Aug 11 2022, 5:58 AM

LGTM - cheers!

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
6420

Please commit this separately referencing the phab patch where it was added (D130782?)

13226

Extendee.getValueType()

peterwaller-arm marked 2 inline comments as done.Aug 11 2022, 7:43 AM

I will submit first thing next week unless there are further comments.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
6420

Done, and a backport requested in https://github.com/llvm/llvm-project/issues/57089 since this case went into LLVM-15-rc3.

This revision was automatically updated to reflect the committed changes.
peterwaller-arm marked an inline comment as done.