[PATCH] D41467: PR35710: Nary reassociation falls into infinite loop

[Evgeny Stupachenko via Phabricator via llvm-commits]

evstupac added inline comments.


================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:466
                                                           BinaryOperator *I) {
+  // Need to avoid special case (C1*v)*(C2*v) = 0 because of overflow and
+  // there is Instruction "mul x, 0", as it leads to infinite loop
----------------
hfinkel wrote:
> evstupac wrote:
> > hfinkel wrote:
> > > If I hadn't read the description, I'd have no idea what this meant (and I'm still not entirely sure). How does this detect overflow? Is it fair to say that x*0 is a special case because it can be  replicated arbitrarily-many times in products (because it is equal to 0, and thus, so is any product of which it is a part), and that can lead to infinite loops.
> > The change is about to limit current implementation, which works fine with zeros. However, it does not expect to see 0 as a result of 2 non-zero expressions product (LHSExpr here).
> > 
> What doesn't expect to see a zero as the result of two non-zero expressions? And here we're also checking that RHS is foldable to zero? Which two expressions are non-zero?
comment on lines 445, 446 to support my answers below:
    // I = (A op B) op RHS
    //   = (A op RHS) op B or (B op RHS) op A

>What doesn't expect to see a zero as the result of two non-zero expressions?
LHSExpr which is product of BExpr, RHSExpr for the case.

>And here we're also checking that RHS is foldable to zero?
Yes. RHS is "A" for the case.

>Which two expressions are non-zero?
RHSExpr and BExpr are non-zero.

Please look at my Dec 21 comment. I've tried to explain the case there using source code names.

Thinking more about this I think it will be correct to exit earlier at line 443 if RHS is foldable to zero and instruction is multiplication. I can update patch that way. Whatever you prefer more.


Repository:
  rL LLVM

https://reviews.llvm.org/D41467