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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri 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();
----------------
benshi001 wrote:
> 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?
> 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. 

I agree that `isLegalAddImmediate()` takes int64_t, but that is orthogonal to the question.

The question being: when doing `int64_t C1 = C1Node->getSExtValue();`,
is it guaranteed that `APInt` value in `C1Node` is at most 64-bit?
If not, this code will crash with an assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83153