[PATCH] D141850: [SCEV] Preserve divisibility and min/max information in applyLoopGuards

[Alon Kom via Phabricator via llvm-commits]

alonkom added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15078
+    // Apply divisibilty by \p Divisor on MinMaxExpr with constant values,
+    // recursively. This is done by aligning up/down the constant value to the
+    // Divisor.
----------------
mkazantsev wrote:
> If you see any problems with compile time from this patch, this is a potential place where things can be limited. Not sure if it's worth it in practice.
> 
I don't expect more than 3 assumes per value in the real-world, so I don't believe it'll affect compilation time drastically, but let's see.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15112
+          const SCEV *RewrittenLHS =
+              I != RewriteMap.end() ? I->second : LHSUnknown;
+          RewrittenLHS = ApplyDivisibiltyOnMinMaxExpr(RewrittenLHS, URemRHS);
----------------
mkazantsev wrote:
> Is it possible that the map contains both `LHSUnknown -> RewrittenLHS ` and `RewrittenLHS  -> SomeOtherLHS`? Should this be a loop?
> 
> I'm ok if it's a separate patch.
The entire function assumes 1 level of nesting


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

https://reviews.llvm.org/D141850