[PATCH] D133768: [DAGCombine] Do not fold SRA/SRL of MUL into MULH when MUL's LSB are used, and MUL_LOHI is available

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9263
+  unsigned MulLoHiOp = IsSignExt ? ISD::SMUL_LOHI : ISD::UMUL_LOHI;
+  if (ShiftOperand->use_size() > 1 &&
+      TLI.isOperationLegalOrCustom(MulLoHiOp, NarrowVT) &&
----------------
Probably should use !hasOneUse instead of computing use_size


================
Comment at: llvm/test/CodeGen/AMDGPU/mul_lohi.ll:1
+; RUN: llc -march=amdgcn -mcpu=gfx900 < %s | FileCheck -check-prefix=GCN %s
+
----------------
Test name is too general for what this is testing


================
Comment at: llvm/test/CodeGen/AMDGPU/mul_lohi.ll:3
+
+define i32 @kernel(i32 %0, i32 %1, i32* %2) {
+  ; GCN-LABEL: kernel:
----------------
This isn't a kernel, rename function


================
Comment at: llvm/test/CodeGen/AMDGPU/mul_lohi.ll:5-7
+  ; GCN:       ; %bb.0:
+  ; GCN-NOT:   v_mul_{{lo|hi}}
+  ; GCN:       v_mad_u64_u32
----------------
Might as well generate these checks


================
Comment at: llvm/test/CodeGen/AMDGPU/mul_lohi.ll:8-9
+  ; GCN:       v_mad_u64_u32
+  %4 = zext i32 %0 to i64
+  %5 = zext i32 %1 to i64
+  %6 = mul nuw i64 %5, %4
----------------
Should use named values in test


================
Comment at: llvm/test/CodeGen/AMDGPU/mul_lohi.ll:16
+  ret i32 %9
+}
----------------
Can you also add a case with a type with illegal mul_lohi, a vector case, and also a shift with a non-constant case, and an off by one shift amount?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133768