[PATCH] D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe

[Warren Ristow via Phabricator via llvm-commits]

wristow marked an inline comment as done.
wristow added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:852
+    // Division by non-zero constants are safe to hoist.
+    IsSafeToHoist = !SC->getValue()->isZero();
   }
----------------
mkazantsev wrote:
> Limiting to constants looks super-conservative. How about getting SCEV for RHS and then asking SE `isKnownPredicate(ICMP_NE, RHS, 0)`?
> 
Sounds good.

That said, I've put this work on the back-burner for the moment, because the effectiveness of this patch depends on the change of rL347934 being in place, and that commit was reverted today (at rL348426), due to SEGVs happening in PHINode Simplification in Instcombine.  That //may //be the same underlying problem that motivated the revert I previously mentioned that was done about a year ago:
    Reverts r289412. It caused an OOB PHI operand access in instcombine when
    ASan is enabled. Reduction in progress.

In any case, until the fix that was done as rL347934 is re-instated in some form, I'll hold off on making the fix discussed here.


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

https://reviews.llvm.org/D55232