[PATCH] D149423: [ValueTracking] Use knownbits interface for determining if `div`/`rem` are safe to speculate

Nikita Popov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 13 04:00:48 PDT 2023


nikic added a comment.

Okay, so as I understand it, this patch ran into two separate problems:

The first is that isSafeToSpeculativeExecute() is used for two different purposes: One is to speculate the instruction (i.e. execute it where it may not have previously been executed, keeping the same arguments), and the other is to execute the instruction at the same position but with different operands (with the implicit assumption that constant operands stay the same). Previously, the same function worked for both of those, but this is no longer the case if we analyze variables.

Actually, I think this is not quite true and we have a pre-existing issue for loads. Consider this example: https://alive2.llvm.org/ce/z/weHUyL If there were a control flow exit between the load and the select, this might speculate a trapping load.

With this in mind, this is clearly something that needs to be solved. My preference would to provide two separate functions (with a shared implementation) for the two different use cases, rather than specifying a flag everywhere. For example, one stays isSafeToSpeculativelyExecute() and the other becomes isSafeToExecuteWithDifferentOperands(). I think we can do this independently of the div change, with the load case as the motivating example.

The second problem is related to the poison check and the context instruction. As far as I understand, the issue are callers of isSafeToSpeculativelyExecute() which don't speculate instructions one-by-one (as LICM does), but rather check a whole sequence for speculability first and then perform the actual transform. This ends up being problematic, because we perform poison queries on instructions in their previous positions, where the fact that they are passed to the division itself implies that they cannot be poison. So what we really want to determine here is "is this instruction non-poison at this context under the assumption that it has already been hoisted there", which is not what the existing APIs do.

This problem is not as simple as not doing the programUndefinedIfUndefOrPoison() check, it can also occur in other contexts. Let's say we have a sequence like this:

  %y = load i32, ptr %p, !noundef !{}, !range !{i32 1, i32 10}
  %d = udiv i32 %x, %y

Even if we ignore that `%y` is non-poison due to being used in the udiv, we also know that it is non-poison due to the `!noundef` metadata. However, that `!noundef` metadata would be dropped if the load were actually speculated. So in this case, the `udiv` is actually not safe to speculate, because `%y` may be poison after speculation.

The two possible ways of solving this I can see are:

1. Add an extra argument to isGuaranteedNotToBeUndefOrPoison() that pretends instructions have been speculated.
2. Only support cases where the div arguments dominate the context instruction (if no context, that would effectively limit to constants/arguments). In that case the plain isGuaranteedNotToBeUndefOrPoison() should work fine.

In D149423#4402529 <https://reviews.llvm.org/D149423#4402529>, @goldstein.w.n wrote:

> Is this still worth pursing?

Not sure, to be honest. I liked this conceptually, but the problem turned out to be much more complicated than anticipated.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2787
+      // Note: Using context-free form to avoid compile time blow up. Likewise
+      // don't set 'PreservesOpCharacteristics' to keep less expensive analysis.
+      if (!isSafeToSpeculativelyExecute(NextInst,
----------------
I don't think we should worry about that here and specify whatever is correct semantically.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1249
+  if (!I || !I->hasOneUse() ||
+      !isSafeToSpeculativelyExecute(I, /* PreservesOpCharacteristics */ true))
     return false;
----------------
This should be `false`.


================
Comment at: llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp:300
+        isSafeToSpeculativelyExecute(&I,
+                                     /* PreservesOpCharacteristics */ false) &&
         AllPrecedingUsesFromBlockHoisted(&I)) {
----------------
Why false here?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:450
   // only uses stuff defined outside of the condition.  If so, hoist it out.
-  if (!isSafeToSpeculativelyExecute(I))
+  if (!isSafeToSpeculativelyExecute(I, /* PreservesOpCharacteristics */ false))
     return false;
----------------
Why false here? This should be simple speculation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149423



More information about the llvm-commits mailing list