[PATCH] D28200: [BypassSlowDivision] Do not bypass division of hash-like values

[Nikolai Bozhenov via Phabricator via llvm-commits]

n.bozhenov marked 2 inline comments as done.
n.bozhenov added inline comments.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:230
+      return true;
+    Visited.insert(V);
+    return llvm::all_of(cast<PHINode>(I)->incoming_values(), [&](Value *V) {
----------------
jlebar wrote:
> Sounds like we should call this VisitedPhis?
In the current implementation there are only PHINodes, but in the future it could be extended to cover SELECT instructions as well. Not sure if we should put Phi into the name.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:231
+    Visited.insert(V);
+    return llvm::all_of(cast<PHINode>(I)->incoming_values(), [&](Value *V) {
+      // Loop-unroll sometimes generates PHI-nodes with Undef incoming values.
----------------
jlebar wrote:
> Why the change from any_of to all_of?  You said earlier:
> 
> > This way it would also be safe (no false positives) to increase the search depth and probably make it unlimited.
> 
> I am not sure what you mean by this.  Are you making two claims about this change:
> 
>  a. It's "safe" in that it reduces our false positives, and
>  b. It lets us safely increase the search depth to infinity?
> 
> I don't see how the switch to any_of changes anything with respect to the cost of the recursion.  I agree that pretty much any sane code is not going to have deep recursion here.  But that's not what I'm worried about.  I'm worried about some crazy code that causes us to have quadratic behavior.  *That's* what the depth limit is there to protect against.
> 
> With respect to (a) I see "safety" opposite of how you've described it.  If we declare that something is hash-like, we won't optimize it, and not optimizing is the "safe" thing to do.  Therefore the safe/conservative choice is to declare something as "maybe hash-like".  Therefore any_of is safer / more conservative than all_of.
> 
> I do have a major safety concern here, but it's about how isHashLikeValue is being used in getValueRange.  Regardless of whether we use any_of or all_of here, it is *not* safe to return VALRNG_LONG if isHashLikeValue(V).  If we return VALRNG_LONG, that is a *promise* that the value is long, and we take advantage of this explicit promise.  isHashLikeValue is *not* a promise that the value is long; it's just a heuristic.  So that whole thing needs to change.  Please also add tests to catch this tricky bug.
> 
> (Perhaps you were accidentally working around this bug in getValueRange by using all_of here and that's the source of this confusion.)
> Why the change from any_of to all_of? You said earlier:
> 
> > This way it would also be safe (no false positives) to increase the search depth and probably make it unlimited.
> 
> I am not sure what you mean by this. Are you making two claims about this change:
> 
> a. It's "safe" in that it reduces our false positives, and
> b. It lets us safely increase the search depth to infinity?

Yes, I'm making both of these claims. Previously I considered a PHINode to be hash-like if any of its inputs were hash-like. If I made a search depth unlimited, a multiplication by a long constant would disable bypassing for all divisions reachable from it via a chain of PHINodes. Even if at runtime another input value is fed into those divisions.

But in the current implementation I require all incoming values for a PHINode to be hash-like. It is a much stricter restriction. And because of that, it is safe (conservative) to increase search depth. One multiplication operation cannot poison some far division operation.


> I don't see how the switch to any_of changes anything with respect to the cost of the recursion. I agree that pretty much any sane code is not going to have deep recursion here. But that's not what I'm worried about. I'm worried about some crazy code that causes us to have quadratic behavior. *That's* what the depth limit is there to protect against.

As we stop traversal of the IR after finding first value that is neither PHINode nor MUL/XOR, the number of traversed instructions is small for any non-malicious IR. And the recursion depth is limited by the length of the longest chain of consecutive PHINodes.


> With respect to (a) I see "safety" opposite of how you've described it. If we declare that something is hash-like, we won't optimize it, and not optimizing is the "safe" thing to do. Therefore the safe/conservative choice is to declare something as "maybe hash-like". Therefore any_of is safer / more conservative than all_of.

My point is that without this patch all long divisions are bypassed. And generally it is a very useful optimization. But in some rare cases this optimization is useless (not harmful) and we would like to disable it. Such disabling is quite risky, because we may get significant performance degradation not bypassing a wrong division. So, here "safety" means "bypass when in doubt".

> I do have a major safety concern here, but it's about how isHashLikeValue is being used in getValueRange. Regardless of whether we use any_of or all_of here, it is *not* safe to return VALRNG_LONG if isHashLikeValue(V). If we return VALRNG_LONG, that is a *promise* that the value is long, and we take advantage of this explicit promise. isHashLikeValue is *not* a promise that the value is long; it's just a heuristic. So that whole thing needs to change. Please also add tests to catch this tricky bug.
> 
> (Perhaps you were accidentally working around this bug in getValueRange by using all_of here and that's the source of this confusion.)

Actually, the comment at VALRNG_LONG declaration says that it is not a promise. VALRNG_LONG means that a value is *unlikely* to fit into the shorter type. If we needed a promise, we would use ValueTracking only and would never introduce an additional analysis in isHashLikeValue. But for this particular optimization it doesn't matter if a value is *guaranteed* to be long or if it is just *likely* to be long.



https://reviews.llvm.org/D28200