[PATCH] D83153: [DAGCombiner] Prevent regression in isMulAddWithConstProfitable
Sanjay Patel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jul 4 08:35:54 PDT 2020
spatel 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();
----------------
How do we know these constants are not wider than 64-bits?
================
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;
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83153/new/
https://reviews.llvm.org/D83153
More information about the llvm-commits
mailing list