[PATCH] D129890: [SDAG] narrow truncated sign_extend_inreg

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 2 inline comments as done.
spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13138
+  // trunc (sign_ext_inreg X, iM) to iN --> sign_ext_inreg (trunc X to iN), iM
+  if (!LegalTypes && N0.getOpcode() == ISD::SIGN_EXTEND_INREG &&
+      N0.hasOneUse()) {
----------------
arsenm wrote:
> Shouldn't really require legal types, but the DAG is lacking in adequate legality checks for sext_inreg
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.


================
Comment at: llvm/test/CodeGen/AMDGPU/mul_int24.ll:184-186
+; SI-NEXT:    v_mul_i32_i24_e32 v3, v0, v2
+; SI-NEXT:    v_mul_hi_i32_i24_e32 v1, v0, v2
+; SI-NEXT:    v_mov_b32_e32 v0, v3
----------------
arsenm wrote:
> This is a pretty big improvement
Thanks for confirming!


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

https://reviews.llvm.org/D129890