[PATCH] D131503: [DAGCombine] Combine signext_inreg of extract-extend

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

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



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13170
+      SDValue SignExtExtendee =
+          DAG.getNode(ISD::SIGN_EXTEND, SDLoc(N), InnerExtVT, Extendee);
+      return DAG.getNode(ISD::EXTRACT_SUBVECTOR, SDLoc(N), VT, SignExtExtendee,
----------------
peterwaller-arm wrote:
> RKSimon wrote:
> > Does this need an operation legality check for ISD::SIGN_EXTEND ? `(!LegalOperations || TLI.isOperationLegal(ISD::SIGN_EXTEND, InnerExtVT))`?
> 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.
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131503/new/

https://reviews.llvm.org/D131503