[PATCH] D83153: [DAGCombiner] Prevent regression in isMulAddWithConstProfitable

[Sanjay Patel via Phabricator via llvm-commits]

spatel added subscribers: fhahn, efriedma, dmgreen.
spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15755-15759
+        // This transform will introduce regression, if c1 is legal add
+        // immediate while c1*c2 isn't.
+        const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+        if (TLI.isLegalAddImmediate(C1) && !TLI.isLegalAddImmediate(C1C2))
+          return false;
----------------
benshi001 wrote:
> benshi001 wrote:
> > spatel wrote:
> > > lebedev.ri wrote:
> > > > You also need a second piece if the puzzle, because we would have
> > > > already performed this transform before ever getting here:
> > > > https://godbolt.org/z/TE_bzk
> > > > 
> > > > You need to add an inverse transform into DAGCombiner.cpp.
> > > Right - I asked if the motivation for this was GEP math:
> > > http://lists.llvm.org/pipermail/llvm-dev/2020-July/142979.html
> > > ...but I did not see an answer.
> > > 
> > > If yes, then we should have GEP tests. If not, then this patch isn't enough.
> > No. I target for normal IR transform, not GEP.
> > 
> > So I need to do a inverse transform?
> Where is the other place perform the same transform before reach here?
The general transform is happening in IR in instcombine, so for example the RISC-V tests are not canonical IR. That means we will not see the pattern that you are expecting in the usual case for "mul (add X, C1), C2" in source code. You can test that by compiling from source code to IR (and on to asm) for something like this:


```
$ cat mad.c
#include <stdint.h>
uint64_t f(uint64_t x) {
  return (x + 424242) * 15700;
}
$ clang -O1 mad.c -S -o - -emit-llvm
define i64 @f(i64 %x) {
  %t0 = mul i64 %x, 15700
  %mul = add i64 %t0, 6660599400
  ret i64 %mul
}

```


================
Comment at: llvm/test/CodeGen/AArch64/urem-seteq-nonzero.ll:7-10
+; CHECK-NEXT:    mov w9, #43691
+; CHECK-NEXT:    sub w8, w0, #1 // =1
+; CHECK-NEXT:    movk w9, #43690, lsl #16
+; CHECK-NEXT:    mul w8, w8, w9
----------------
I think we need to hear from someone with more AArch knowledge if this is an improvement or acceptable. cc @dmgreen @efriedma @fhahn 


================
Comment at: llvm/test/CodeGen/RISCV/addimm-mulimm.ll:74
-
-define signext i32 @add_mul_trans_reject_2(i32 %x) {
-; RV32IM:       # %bb.0:
----------------
Why is this test deleted?


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

https://reviews.llvm.org/D83153