[PATCH] D17553: [SimplifyCFG] Speculatively flatten CFG based on profiling metadata (try 2)

[Philip Reames via llvm-commits]

reames added a comment.

Responding only to the material design comment for the moment.  Will address nits once the design question is resolved.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2759
@@ +2758,3 @@
+    if (auto *OBO = dyn_cast<OverflowingBinaryOperator>(I))
+      if (OBO->hasNoUnsignedWrap() || OBO->hasNoSignedWrap())
+        // TODO: can we prove the flags are valid before PBI?
----------------
sanjoy wrote:
> I'd say this is a little risky.  I think you're okay now since `isImpliedCondition` only looks at overflow flags, but if it is changed to consider, e.g., the `exact` flag on `udiv` or `inbounds` on `getelementptr` at some time in the future, this might subtly break.
> 
> I'd rather split out a helper routine, `GetCombinedCond` that takes two predicates, `P` and `Q` and a location, `Loc`, and returns a predicate that is equivalent to `P && Q` at `Loc`, if it can be cheaply computed.  Predicates like `5 u< L` and `6 u< L` can be combined into `6 u< L`; and predicates like `I u< L` and `(I +nuw 1) u< L` can be combined into `(I +nuw 1) u< L` if you can prove that the `nuw` holds at `Loc`, etc.
To be clear, I'm not proposing that we *just* check nsw/nuw.  As you point out, that would be utterly unsound.  Assuming I extend the logic to include exact and inbounds (which I had completely forgotten about), what do you think of the approach?  (If I've forgotten other cases, please suggest them.)

I'm a bit leery of introducing the separate predicate.  I think that unless we're really careful here, we could end up duplicating a lot of code.  Now, having said that, I'll give it a bit more thought tomorrow and see what I think after writing some prototype code.  


http://reviews.llvm.org/D17553