[ValueTracking] Early out of isValidAssumeForContext if the assume and the context instruction are the same

[craig.topper]

Summary

There are two loops in this function that start at the instruction immediately after the context instruction and scans forward until it encounters the assume or another condition. But if the assume and the context are the same instruction these loops can never find the assume.

Eventually we'll end up in the second loop which will return false when isSafeToSpeculativelyExecute encounters the terminator of the basic block and returns false. But that's seems like a lot of extra looping to get that result.

This was found in asm-goto testing because we failed to add the new CallBr terminator to isSafeToSpeculativelyExecute.

[efriedma]

Is it possible to write a testcase without the callbr patch?

lib/Analysis/ValueTracking.cpp

549

I think we should return "true" here? The condition of an assume should hold at the point the assume executes.

[craig.topper]

With context

[craig.topper]

If we return false like we currently do via the loop. Then I don't think it can be tested since its really just making explicit what was previously an odd iteration behavior. If we change it to true then maybe that would be a behavior change we could see.

lib/Analysis/ValueTracking.cpp

549

I just did what the existing behavior ended up with when they hit the end of the basic block and called isSafeToSpeculativelyExecute

[efriedma]

If we're going to do anything here, I'd prefer to change it to true, since that's the right answer... it seems like it isn't doing any harm in the meantime.

[hfinkel]

In D56867#1362336, @efriedma wrote:

If we're going to do anything here, I'd prefer to change it to true, since that's the right answer... it seems like it isn't doing any harm in the meantime.

As I recall, it needs to be false. Otherwise, an assumption can imply the truth of its own argument and it will cause its own trivialization (and removal).

[efriedma]

Otherwise, an assumption can imply the truth of its own argument and it will cause its own trivialization (and removal).

I think the isEphemeralValueOf check is supposed to take care of that? (The function currently allows cases where the context instruction is before the llvm.assume call.)

lib/Analysis/ValueTracking.cpp

bool llvm::isValidAssumeForContext(const Instruction *Inv,
   if (DT) {
     if (DT->dominates(Inv, CxtI))
       return true;
   } else if (Inv->getParent() == CxtI->getParent()->getSinglePredecessor()) {
     // We don't have a DT, but this trivially dominates.
     return true;
   }
 
+  // If the context instruction is this assume, we shouldn't consider it valid.
+  // Otherwise the loops below will scan forward to the end of the block.
+  if (Inv == CxtI)
+    return false;
+
   // With or without a DT, the only remaining case we will check is if the
   // instructions are in the same BB.  Give up if that is not the case.
   if (Inv->getParent() != CxtI->getParent())
     return false;
 
   // If we have a dom tree, then we now know that the assume doesn't dominate
   // the other instruction.  If we don't have a dom tree then we can check if
   // the assume is first in the BB.