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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Generally looks good, can you also add a test which will trigger an overflow with your previous revision?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15750
+    // are too large.
+    unsigned Bits = ConstNode.getScalarValueSizeInBits();
+    if (Bits > 8 * sizeof(int64_t))
----------------
Nit: const unsigned Bits


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15757
+        const APInt &C2 = C2Node->getAPIntValue();
+        // Prevent the transform since c1*c2 is overflow.
+        if ((C1 * C2).getBitWidth() > ConstNode.getScalarValueSizeInBits())
----------------
// Prevent the transform if c1*c2 overflows.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15758
+        // Prevent the transform since c1*c2 is overflow.
+        if ((C1 * C2).getBitWidth() > ConstNode.getScalarValueSizeInBits())
+          return false;
----------------
ConstNode.getScalarValueSizeInBits() -> Bits


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15761
+        // Do sign extension for c1*c2 according to c2's type.
+        int64_t C1C2 = llvm::SignExtend64((C1 * C2).getZExtValue(), Bits);
+        // This transform will introduce regression, if c1 is legal add
----------------
Store `C1 * C2` in a variable. Please don't repeat multiplication.


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

https://reviews.llvm.org/D83153