This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Don't undo 0 - (X * Y) canonicalization when combining subs.
ClosedPublic

Authored by fhahn on Jan 14 2019, 11:31 AM.

Details

Summary

Otherwise instcombine gets stuck in a cycle. The canonicalization was
added in D55961.

This patch fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12400

Diff Detail

Event Timeline

fhahn created this revision.Jan 14 2019, 11:31 AM
fhahn retitled this revision from [InstCombine] Avoid undoing 0 - (X * Y) canonicalization when combining subs. to [InstCombine] Don't undo 0 - (X * Y) canonicalization when combining subs..
fhahn edited the summary of this revision. (Show Details)

We are sure that we want to break the cycle here, and not at the other end?
This results in the optimal IR?

test/Transforms/InstCombine/mul.ll
505–507

Can you please add an explanation here?

We are sure that we want to break the cycle here, and not at the other end?
This results in the optimal IR?

I think it's fine to break the cycle here, but we should address the problem pattern directly - we want to avoid the transform on a constant expression rather than a plain constant.

So something like this (the variable named "CI" is misleading since that usually refers to "ConstantInt"):

Index: lib/Transforms/InstCombine/InstCombineAddSub.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineAddSub.cpp	(revision 351077)
+++ lib/Transforms/InstCombine/InstCombineAddSub.cpp	(working copy)
@@ -1649,7 +1649,6 @@
     // X - A*-B -> X + A*B
     // X - -A*B -> X + A*B
     Value *A, *B;
-    Constant *CI;
     if (match(Op1, m_c_Mul(m_Value(A), m_Neg(m_Value(B)))))
       return BinaryOperator::CreateAdd(Op0, Builder.CreateMul(A, B));
 
@@ -1656,8 +1655,8 @@
     // X - A*CI -> X + A*-CI
     // No need to handle commuted multiply because multiply handling will
     // ensure constant will be move to the right hand side.
-    if (match(Op1, m_Mul(m_Value(A), m_Constant(CI)))) {
-      Value *NewMul = Builder.CreateMul(A, ConstantExpr::getNeg(CI));
+    if (match(Op1, m_Mul(m_Value(A), m_Constant(C))) && !isa<ConstantExpr>(C)) {
+      Value *NewMul = Builder.CreateMul(A, ConstantExpr::getNeg(C));
       return BinaryOperator::CreateAdd(Op0, NewMul);
     }
   }

We needed a similar fix here:
rL333610

fhahn updated this revision to Diff 181627.Jan 14 2019, 12:18 PM

Thanks, added check for ConstantExpr.

fhahn marked an inline comment as done.Jan 14 2019, 12:18 PM
fhahn marked an inline comment as done.Jan 14 2019, 12:25 PM
fhahn added inline comments.
test/Transforms/InstCombine/mul.ll
448

I re-run update_test_checks.py and it changed those lines. Let me know if I should undo the unrelated changes.

spatel accepted this revision.Jan 14 2019, 1:16 PM

LGTM

lib/Transforms/InstCombine/InstCombineAddSub.cpp
1656–1657

Please change the 'CI' to 'C' in this comment to match the updated code.

This revision is now accepted and ready to land.Jan 14 2019, 1:16 PM

Thanks for the fix.

Looks good, no further comments from me.

fhahn closed this revision.Jan 15 2019, 3:27 AM

Committed in rL351187, thanks!