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

[Ben Shi via Phabricator via llvm-commits]

benshi001 marked 3 inline comments as done.
benshi001 added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15750-15753
+        int64_t C1 = C1Node->getSExtValue();
+        int64_t C2 = C2Node->getSExtValue();
+        // Do sign extension for c1*c2 according to c2's type.
+        int ShiftBits = 64 - ConstNode.getScalarValueSizeInBits();
----------------
spatel wrote:
> How do we know these constants are not wider than 64-bits?
In real world, no machines will encode a large imm into instruction. Even x86 support legalAddImm up to 32-bit, and arm & riscv only support to 12-bit. So I think using int64_t here is OK. If the real value exceed that, then the DAG falls to normal path --- get transformed.

What's your opinion?


================
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;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83153