LICM: Do not sink or hoist assume intrinsic calls.

[pcc]

Summary

This isn't useful, and it may result in assumes being removed.

This deals with more fallout from the AA change from rL268068.

[sanjoy]

IIUC this is also a correctness fix, right?

Since you could have had:

volatile boolean always_false = ...;
for (...)
  if (always_false)
    assume(false);

and hoisting out assume(false) to the loop preheader will make the loop appear dead.

[sanjoy]

Never mind, I was mistaken; you can't hoist out a readnone nounwind function out of control flow anyway, and the same logic should hold for assumes.

[hfinkel]

In D20653#440268, @sanjoy wrote:

IIUC this is also a correctness fix, right?

Since you could have had:

volatile boolean always_false = ...;
for (...)
  if (always_false)
    assume(false);

and hoisting out assume(false) to the loop preheader will make the loop appear dead.

Yes.

I was afraid something like this might happen. I think we need to revert r268068 (and the various follow-up fixes). Thoughts?

[• dberlin]

Can we just make assume use the same machinery we are now using for control
flow dependencies rather than continued a special case it everywhere

[hfinkel]

In D20653#440276, @sanjoy wrote:

Never mind, I was mistaken; you can't hoist out a readnone nounwind function out of control flow anyway, and the same logic should hold for assumes.

Ah. Because of potential UB?

[• dberlin]

I'm fine with reverting it if we aren't going to fix it properly ATM.

But of course, for the moment that just means special casing assume in
other places

[hfinkel]

In D20653#440281, @dberlin wrote:

Can we just make assume use the same machinery we are now using for control
flow dependencies rather than continued a special case it everywhere

Do you mean for convergent?

[pcc]

Personally I'd be more comfortable with reverting. If we do need special casing at this point, it seems a little safer to special case to allow optimizations than to special case to inhibit them.

[sanjoy]

In D20653#440282, @hfinkel wrote:

In D20653#440276, @sanjoy wrote:

Never mind, I was mistaken; you can't hoist out a readnone nounwind function out of control flow anyway, and the same logic should hold for assumes.

Ah. Because of potential UB?

Yes, an implementation of assume could be void assume(bool cond) { if (!cond) 1/0; }.

[hfinkel]

In D20653#440288, @sanjoy wrote:

In D20653#440282, @hfinkel wrote:

In D20653#440276, @sanjoy wrote:

Never mind, I was mistaken; you can't hoist out a readnone nounwind function out of control flow anyway, and the same logic should hold for assumes.

Ah. Because of potential UB?

Yes, an implementation of assume could be void assume(bool cond) { if (!cond) 1/0; }.

Yes, but don't we normally assume that this UB concern does not apply to intrinsics?

[• dberlin]

Granted, but in reality, we want neither ;)

[sanjoy]

In D20653#440289, @hfinkel wrote:

In D20653#440288, @sanjoy wrote:

In D20653#440282, @hfinkel wrote:

In D20653#440276, @sanjoy wrote:

Never mind, I was mistaken; you can't hoist out a readnone nounwind function out of control flow anyway, and the same logic should hold for assumes.

Ah. Because of potential UB?

Yes, an implementation of assume could be void assume(bool cond) { if (!cond) 1/0; }.

Yes, but don't we normally assume that this UB concern does not apply to intrinsics?

I wasn't aware of such a restriction -- can you spell it out explicitly? We do have intrinsics like patchpoint and statepoint that can execute arbitrary code, so stating that intrinsics have no UB at all is probably not okay.

[hfinkel]

In D20653#440307, @sanjoy wrote:

In D20653#440289, @hfinkel wrote:

In D20653#440288, @sanjoy wrote:

In D20653#440282, @hfinkel wrote:

In D20653#440276, @sanjoy wrote:

Never mind, I was mistaken; you can't hoist out a readnone nounwind function out of control flow anyway, and the same logic should hold for assumes.

Ah. Because of potential UB?

Yes, an implementation of assume could be void assume(bool cond) { if (!cond) 1/0; }.

Yes, but don't we normally assume that this UB concern does not apply to intrinsics?

I wasn't aware of such a restriction -- can you spell it out explicitly? We do have intrinsics like patchpoint and statepoint that can execute arbitrary code, so stating that intrinsics have no UB at all is probably not okay.

Good point, although those aren't marked as readnone. I'm quite possibly mistaken on this point, although it seems like we should have some kind of distinction on this front since most readnone intrinsics don't have value-dependent UB. We might need a safe-to-speculate attribute or something like that, however.

[pcc]

D20653 is the revert.

[pcc]

Sorry, I meant D20658

[pcc]

Obsoleted by D20658

lib/Transforms/Scalar/LICM.cpp

if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
     if (LI->getType()->isSized())
       Size = I.getModule()->getDataLayout().getTypeStoreSize(LI->getType());
 
     AAMDNodes AAInfo;
     LI->getAAMetadata(AAInfo);
 
     return !pointerInvalidatedByLoop(LI->getOperand(0), Size, AAInfo, CurAST);
   } else if (CallInst *CI = dyn_cast<CallInst>(&I)) {
-    // Don't sink or hoist dbg info; it's legal, but not useful.
-    if (isa<DbgInfoIntrinsic>(I))
+    // Don't sink or hoist dbg info or assumes; it's legal, but not useful.
+    if (auto *II = dyn_cast<IntrinsicInst>(&I)) {
+      if (isa<DbgInfoIntrinsic>(II) ||
+          II->getIntrinsicID() == Intrinsic::assume)
         return false;
+    }
 
     // Don't sink calls which can throw.
     if (CI->mayThrow())
       return false;
 
     // Handle simple cases by querying alias analysis.
     FunctionModRefBehavior Behavior = AA->getModRefBehavior(CI);
     if (Behavior == FMRB_DoesNotAccessMemory)

test/Transforms/LICM/assume.ll

+; RUN: opt -licm -basicaa < %s -S | FileCheck %s
+
+define void @f(i1 %p) nounwind ssp {
+entry:
+  br label %for.body
+
+for.body:
+  br i1 undef, label %if.then, label %for.cond.backedge
+
+for.cond.backedge:
+  br i1 undef, label %for.end104, label %for.body
+
+if.then:
+  br i1 undef, label %if.then27, label %if.end.if.end.split_crit_edge.critedge
+
+if.then27:
+; CHECK: tail call void @llvm.assume
+  tail call void @llvm.assume(i1 %p)
+  br label %for.body61.us
+
+if.end.if.end.split_crit_edge.critedge:
+  br label %for.body61
+
+for.body61.us:
+  br i1 undef, label %for.cond.backedge, label %for.body61.us
+
+for.body61:
+  br i1 undef, label %for.cond.backedge, label %for.body61
+
+for.end104:
+  ret void
+}
+
+declare void @llvm.assume(i1)