Strip undef implying attributes when moving calls

[anna]

Summary

When hoisting/moving calls to locations, we strip unknown metadata. Such calls are usually marked speculatable, i.e. they are guaranteed to not cause undefined behaviour when run anywhere. So, we should strip attributes that can cause immediate undefined behaviour if those attributes are not valid in the context where the call is moved to.

Fix for PR50744.

[anna]

updated comments

[lebedev.ri]

Should this not be integrated into Instruction::hasMetadataOtherThanDebugLoc()/Instruction::dropUnknownNonDebugMetadata() ?

[anna]

In D104641#2830481, @lebedev.ri wrote:

Should this not be integrated into Instruction::hasMetadataOtherThanDebugLoc()/Instruction::dropUnknownNonDebugMetadata() ?

I'm not too sure frankly. There's two differences, so if we integrate it, we will need something to reflect that in the name:

1. Attribute stripping is only for calls. On other instructions, attributes are needed for correctness (while metadata is just for performance and can be stripped without any impact on correctness).
2. As mentioned in #1, there are attributes that are needed for correctness. So, we keep a whitelist of attributes we can legally strip from the call parameters.

On the other hand though, it makes sense to strip such attributes not just in hoisting, but also when instructions are "moved"/cloned to another location (such as stripping metadata done in SimplifyCFG). So, I can see a case for integrating this into the functionality where we drop unknown metadata.

[asbirlea]

It may be too much to overload the existing APIs, since this applies only to calls (e.g. in GVNHoist, metadata is dropped for geps, this would be unnecessary). So either doing the two calls independently as in this patch or add another API that does both.
I don't have strong opinions either way.

Is it possible this patch applies at the call in SimplifyCFG.cpp:1092 and :2487 and at hoistAllInstructionsInto in Local.cpp (also used by SimplifyCFG)? If the number of use cases is large enough, an API with both might make sense.

Side note, I believe there are cases where metadata must be dropped for correctness as well.

[anna]

In D104641#2831539, @asbirlea wrote:

It may be too much to overload the existing APIs, since this applies only to calls (e.g. in GVNHoist, metadata is dropped for geps, this would be unnecessary). So either doing the two calls independently as in this patch or add another API that does both.
I don't have strong opinions either way.

Is it possible this patch applies at the call in SimplifyCFG.cpp:1092 and :2487 and at hoistAllInstructionsInto in Local.cpp (also used by SimplifyCFG)? If the number of use cases is large enough, an API with both might make sense.

It looks like both cases in SimplifyCFG and hoistAllInstructionsInto are applicable (will try constructing test cases). Also, another spot is makeLoopInvariant in Analysis/LoopInfo.cpp. There we can make a readnone speculatable call loop-invariant (and hoist it to specified point outside loop).

Side note, I believe there are cases where metadata must be dropped for correctness as well.

Ah, I meant metadata can be dropped unconditionally and that will not impact correctness. Performance can be affected if we dropped metadata that could be retained.
Whereas for attributes, if we drop them unconditionally, it can affect correctness.

I agree to your point - actually, in most cases, we drop metadata to preserve correctness.

[nikic]

llvm/lib/IR/Instructions.cpp

353 ↗

(On Diff #353358)

We already have removeParamUndefImplyingAttributes() with a similar but not same list of attributes. I'm not sure if the desired semantics line up, but if they do it would be good to extend/reuse that.

[jdoerfert]

llvm/lib/IR/Instructions.cpp

350 ↗

(On Diff #353358)

We got rid of "whitelist". It is also weird to call it that as we actually remove those. I'm not sure having a list here is great. Should we have a Attribute::isABIrelated() instead?

[anna]

llvm/lib/IR/Instructions.cpp

350 ↗

(On Diff #353358)

Yeah. I think isABIRelated is a good choice here. I'll add that function and use it here, instead of a list. It also tests the theory that only ABI related attributes is illegal to strip.

353 ↗

(On Diff #353358)

Looking at that code piece, I don't think they line up for what's required here. Specifically, it is for updating attributes if a given parameter has undefined/poison behaviour in it.

[anna]

addressed review comments. Strip attributes whenever a call is moved (either through hoisting or simplifyCFG).
See updated tests.

[nikic]

Do I understand correctly that a function declaration like declare void @test(i32 noundef) speculatable is illegal, because the parameter attributes can result in UB? So if the speculatable attribute is present, we can assume that all attributes present on the function declaration are also safe to speculate, and the only thing we have to worry about are additional non-speculatable attributes on the call-site?

llvm/include/llvm/IR/Instruction.h

397

nit: moves -> move

llvm/lib/IR/Attributes.cpp

279

Shouldn't this also include at least inreg, byval, byref, preallocated, inalloca, sret? Possibly also swiftself/swiftasync and alignstack?

Might make sense to update LangRef with a clearer distinction between ABI and optimization attributes for arguments.

279

Though generally, I'm not convinced that we should treat this as dropping all non-abi attributes.

I think we should only be dropping attributes for which passing an invalid value results in immediate undefined behavior. Dropping nonnull for example should not be needed, as that just results in a poison argument, and the speculatable attribute promises that the function must deal with that just fine.

The only attributes we need to drop are things like noundef, dereferenceable and dereferenceable_or_null. Which is pretty much what removeParamUndefImplyingAttributes() is doing (modulo implementation bugs).

llvm/lib/IR/Instruction.cpp

185

What about return attributes? Aren't they also affected?

[anna]

In D104641#2858546, @nikic wrote:

Do I understand correctly that a function declaration like declare void @test(i32 noundef) speculatable is illegal, because the parameter attributes can result in UB?

Yes, that's right.

So if the speculatable attribute is present, we can assume that all attributes present on the function declaration are also safe to speculate, and the only thing we have to worry about are additional non-speculatable attributes on the call-site?

That's one way to reason about it. So, basically we do not need to be as restrictive as the current patch. For example, if we have the speculatable attribute on the function declaration and non null only on the parameter at the callsite, we should be able to run this call anywhere even if the parameter maybe null at the location the call is moved to. The only attributes we should strip are those that cause immediate UB. As Artur pointed out offline, it looks like nonnull behaviour was also changed recently to return poison instead of causing UB (if the pointer is indeed null). Given the speculatable attribute, we should never have UB within that function if we were to pass in a poison argument.

llvm/lib/IR/Attributes.cpp

279

agreed. Thanks for the clarification.

llvm/lib/IR/Instruction.cpp

185

Yes, that's right. We could move the call anywhere (not just hoist it above conditions affecting the parameter attributes).

[anna]

addressed review comments from @nikic.

[anna]

llvm/lib/IR/Attributes.cpp

279

Note that removeParamUndefImplyingAttributes does have Nonnull in it currently and the comment states that it removes attributes that imply both UB and poison.
The API was added here: https://reviews.llvm.org/D98899 and there's a comment stating that removing nonnull is strictly not necessary but doesn't hurt either. After all, we are propagating poison in that speculatable call.

My updated tests also state the same - we need not remove nonnull. I have also added tests to show where we should strictly remove attributes (such as dereferenceable).

[anna]

address typo in failing testcase.

[anna]

ping

[nikic]

I pushed https://github.com/llvm/llvm-project/commit/99f869c8f00a36dac3c774178b69d05876a29a31 and https://github.com/llvm/llvm-project/commit/087a8eea359a4d8ef22c42fddca3b09833928c6a as some preliminary cleanup, could you please rebase? Generally this looks good to me though.

[jdoerfert]

Rebase on 087a8eea359a should make it a bit smaller. I'm unsure why the nonnull is listed in PopulateUndefImplyingAttributes I guess after the rebase it will be gone and the tests that drop it won't anymore.

seems there are no more real concerns, so LGTM if the rebase works out as expected.

llvm/test/Transforms/LICM/call-hoisting.ll

31

[jdoerfert]

In D104641#2903134, @nikic wrote:

I pushed https://github.com/llvm/llvm-project/commit/99f869c8f00a36dac3c774178b69d05876a29a31 and https://github.com/llvm/llvm-project/commit/087a8eea359a4d8ef22c42fddca3b09833928c6a as some preliminary cleanup, could you please rebase? Generally this looks good to me though.

I was looking at the code and missed one of the patches and your comment. Does look like you agree and we can go ahead here though.

[anna]

rebased over landed changes. Use getUBImplyingAttributes API.

[anna]

llvm/lib/Analysis/IVDescriptors.cpp

202 ↗

(On Diff #361787)

Ah. ignore stale comment for fixing another bug.

[anna]

In D104641#2903135, @jdoerfert wrote:

Rebase on 087a8eea359a should make it a bit smaller. I'm unsure why the nonnull is listed in PopulateUndefImplyingAttributes I guess after the rebase it will be gone and the tests that drop it won't anymore.

seems there are no more real concerns, so LGTM if the rebase works out as expected.

Thanks @jdoerfert. I'll give it a day if anyone has concerns over the rebased code.

[anna]

removed unrelated change.

[lebedev.ri]

LGTM, thank you.
Looks like my original design comment was essentially addressed, so i really like this.

It would be best to land this within next ~12 hours or so, before branching happens,
i.e. i'd maybe suggesting landing how.

llvm/include/llvm/IR/Instruction.h

331–334

dropUnknownNonDebugMetadata()'s documentation really needs an update
to point out that dropUndefImplyingAttrsAndUnknownMetadata() likely should be used instead.

398–401

Wouldn't

void dropUndefImplyingAttrsAndUnknownMetadata(ArrayRef<unsigned> KnownIDs = {});

suffice?

[nikic]

LG from my side as well.

[anna]

llvm/include/llvm/IR/Instruction.h

398–401

agreed, done.

llvm/include/llvm/IR/Attributes.h

public:
   }
 
   /// Remove noundef attribute and other attributes that imply undefined
   /// behavior if a `undef` or `poison` value is passed from this attribute
   /// list. Returns a new list because attribute lists are immutable.
   LLVM_NODISCARD AttributeList
   removeParamUndefImplyingAttributes(LLVMContext &C, unsigned ArgNo) const;
 
+  /// Remove noundef attribute and other attributes in return value that imply
+  /// undefined behavior if a `undef` or `poison` value is passed from this
+  /// attribute list. Returns a new list because attribute lists are immutable.
+  LLVM_NODISCARD AttributeList
+  removeUndefImplyingReturnAttributes(LLVMContext &C) const;
+
   /// Remove all attributes at the specified arg index from this
   /// attribute list. Returns a new list because attribute lists are immutable.
   LLVM_NODISCARD AttributeList removeParamAttributes(LLVMContext &C,
                                                      unsigned ArgNo) const {
     return removeAttributes(C, ArgNo + FirstArgIndex);
   }
 
   /// Replace the type contained by attribute \p AttrKind at index \p ArgNo wih

llvm/include/llvm/IR/Instruction.h

public:
   void copyMetadata(const Instruction &SrcInst,
                     ArrayRef<unsigned> WL = ArrayRef<unsigned>());
 
   /// If the instruction has "branch_weights" MD_prof metadata and the MDNode
   /// has three operands (including name string), swap the order of the
   /// metadata.
   void swapProfMetadata();
 
   /// Drop all unknown metadata except for debug locations.
   /// @{
   /// Passes are required to drop metadata they don't understand. This is a
   /// convenience method for passes to do so.
   void dropUnknownNonDebugMetadata(ArrayRef<unsigned> KnownIDs);
   void dropUnknownNonDebugMetadata() {
     return dropUnknownNonDebugMetadata(None);
   }
   void dropUnknownNonDebugMetadata(unsigned ID1) {
     return dropUnknownNonDebugMetadata(makeArrayRef(ID1));
   }
   void dropUnknownNonDebugMetadata(unsigned ID1, unsigned ID2) {
public:
 
   /// Determine whether the no signed wrap flag is set.
   bool hasNoSignedWrap() const;
 
   /// Drops flags that may cause this instruction to evaluate to poison despite
   /// having non-poison inputs.
   void dropPoisonGeneratingFlags();
 
+  /// This function drops non-debug unknown metadata (through
+  /// dropUnknownNonDebugMetadata). For calls, it also drops parameter and 
+  /// return attributes that can cause undefined behaviour. Both of these should
+  /// be done by passes which move instructions in IR.
+  void dropUndefImplyingAttrsAndUnknownMetadata();
+
+  /// Use this API when we need to preserve some metadata through KnownIDs.
+  void dropUndefImplyingAttrsAndUnknownMetadata(ArrayRef<unsigned> KnownIDs);
+
   /// Determine whether the exact flag is set.
   bool isExact() const;
 
   /// Set or clear all fast-math-flags on this instruction, which must be an
   /// operator which supports this flag. See LangRef.html for the meaning of
   /// this flag.
   void setFast(bool B);
 

llvm/lib/IR/Attributes.cpp

 
 bool Attribute::isTypeAttribute() const {
   return pImpl && pImpl->isTypeAttribute();
 }
 
 Attribute::AttrKind Attribute::getKindAsEnum() const {
   if (!pImpl) return None;
   assert((isEnumAttribute() || isIntAttribute() || isTypeAttribute()) &&
          "Invalid attribute type to get the kind as an enum!");
   return pImpl->getKindAsEnum();
 }
 
 uint64_t Attribute::getValueAsInt() const {
   if (!pImpl) return 0;
   assert(isIntAttribute() &&
          "Expected the attribute to be an integer attribute!");
   return pImpl->getValueAsInt();
AttributeList AttributeList::removeAttributes(LLVMContext &C,
   WithoutIndex = attrIdxToArrayIdx(WithoutIndex);
   if (WithoutIndex >= getNumAttrSets())
     return *this;
   SmallVector<AttributeSet, 4> AttrSets(this->begin(), this->end());
   AttrSets[WithoutIndex] = AttributeSet();
   return getImpl(C, AttrSets);
 }
 
-AttributeList
-AttributeList::removeParamUndefImplyingAttributes(LLVMContext &C,
-                                                  unsigned ArgNo) const {
-  AttrBuilder B;
+static void PopulateUndefImplyingAttributes(AttrBuilder &B) {
   B.addAttribute(Attribute::NoUndef);
   B.addAttribute(Attribute::NonNull);
   B.addDereferenceableAttr(1);
   B.addDereferenceableOrNullAttr(1);
+}
+
+AttributeList
+AttributeList::removeParamUndefImplyingAttributes(LLVMContext &C,
+                                                  unsigned ArgNo) const {
+  AttrBuilder B;
+  PopulateUndefImplyingAttributes(B);
   return removeParamAttributes(C, ArgNo, B);
 }
 
+AttributeList
+AttributeList::removeUndefImplyingReturnAttributes(LLVMContext &C) const {
+  AttrBuilder B;
+  PopulateUndefImplyingAttributes(B);
+  return removeAttributes(C, AttributeList::ReturnIndex, B);
+}
+
 AttributeList AttributeList::addDereferenceableAttr(LLVMContext &C,
                                                     unsigned Index,
                                                     uint64_t Bytes) const {
   AttrBuilder B;
   B.addDereferenceableAttr(Bytes);
   return addAttributes(C, Index, B);
 }
 

llvm/lib/IR/Instruction.cpp

void Instruction::dropPoisonGeneratingFlags() {
 
   case Instruction::GetElementPtr:
     cast<GetElementPtrInst>(this)->setIsInBounds(false);
     break;
   }
   // TODO: FastMathFlags!
 }
 
+void Instruction::dropUndefImplyingAttrsAndUnknownMetadata() {
+  dropUndefImplyingAttrsAndUnknownMetadata(None);
+}
+
+void Instruction::dropUndefImplyingAttrsAndUnknownMetadata(
+    ArrayRef<unsigned> KnownIDs) {
+  dropUnknownNonDebugMetadata(KnownIDs);
+  auto *CB = dyn_cast<CallBase>(this);
+  if (!CB)
+    return;
+  // For call instructions, we also need to drop parameter attributes that are
+  // can cause UB if the call is moved to a location where the attribute is not
+  // valid.
+  AttributeList AL = CB->getAttributes();
+  if (AL.isEmpty())
+    return;
+  
+  for (unsigned ArgNo = 0; ArgNo < CB->getNumArgOperands(); ArgNo++) {
+    AL = AL.removeParamUndefImplyingAttributes(CB->getContext(), ArgNo);
+    CB->setAttributes(AL);
+  }
+
+  AL = AL.removeUndefImplyingReturnAttributes(CB->getContext());
+  CB->setAttributes(AL);
+}
 
 bool Instruction::isExact() const {
   return cast<PossiblyExactOperator>(this)->isExact();
 }
 
 void Instruction::setFast(bool B) {
   assert(isa<FPMathOperator>(this) && "setting fast-math flag on invalid op");
   cast<FPMathOperator>(this)->setFast(B);

llvm/lib/Transforms/Scalar/LICM.cpp

ORE->emit([&]() {
     return OptimizationRemark(DEBUG_TYPE, "Hoisted", &I) << "hoisting "
                                                          << ore::NV("Inst", &I);
   });
 
   // Metadata can be dependent on conditions we are hoisting above.
   // Conservatively strip all metadata on the instruction unless we were
   // guaranteed to execute I if we entered the loop, in which case the metadata
   // is valid in the loop preheader.
-  if (I.hasMetadataOtherThanDebugLoc() &&
+  // Similarly, If I is a call and it is not guaranteed to execute in the loop,
+  // then moving to the preheader means we should strip attributes on the call
+  // that can cause UB since we may be hoisting above conditions that allowed
+  // inferring those attributes. They may not be valid at the preheader.
+  if ((I.hasMetadataOtherThanDebugLoc() || isa<CallInst>(I))&&
       // The check on hasMetadataOtherThanDebugLoc is to prevent us from burning
       // time in isGuaranteedToExecute if we don't actually have anything to
       // drop.  It is a compile time optimization, not required for correctness.
       !SafetyInfo->isGuaranteedToExecute(I, DT, CurLoop))
-    I.dropUnknownNonDebugMetadata();
+    I.dropUndefImplyingAttrsAndUnknownMetadata();
 
   if (isa<PHINode>(I))
     // Move the new node to the end of the phi list in the destination block.
     moveInstructionBefore(I, *Dest->getFirstNonPHI(), *SafetyInfo, MSSAU, SE);
   else
     // Move the new node to the destination block, before its terminator.
     moveInstructionBefore(I, *Dest->getTerminator(), *SafetyInfo, MSSAU, SE);
 

llvm/lib/Transforms/Utils/Local.cpp

void llvm::hoistAllInstructionsInto(BasicBlock *DomBlock, Instruction *InsertPt,
   // See PR38762, PR39243 for more details.
   //
   // TODO: Extend llvm.dbg.value to take more than one SSA Value (PR39141) to
   // encode predicated DIExpressions that yield different results on different
   // code paths.
 
   for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE;) {
     Instruction *I = &*II;
-    I->dropUnknownNonDebugMetadata();
+    I->dropUndefImplyingAttrsAndUnknownMetadata();
     if (I->isUsedByMetadata())
       dropDebugUsers(*I);
     if (I->isDebugOrPseudoInst()) {
       // Remove DbgInfo and pseudo probe Intrinsics.
       II = I->eraseFromParent();
       continue;
     }
     I->setDebugLoc(InsertPt->getDebugLoc());

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

for (Instruction &BonusInst : *BB) {
     VMap[&BonusInst] = NewBonusInst;
 
     // If we moved a load, we cannot any longer claim any knowledge about
     // its potential value. The previous information might have been valid
     // only given the branch precondition.
     // For an analogous reason, we must also drop all the metadata whose
     // semantics we don't understand. We *can* preserve !annotation, because
     // it is tied to the instruction itself, not the value or position.
-    NewBonusInst->dropUnknownNonDebugMetadata(LLVMContext::MD_annotation);
+    // Similarly strip attributes on call parameters that may cause UB in
+    // location the call is moved to.
+    NewBonusInst->dropUndefImplyingAttrsAndUnknownMetadata(LLVMContext::MD_annotation);
 
     PredBlock->getInstList().insert(PTI->getIterator(), NewBonusInst);
     NewBonusInst->takeName(&BonusInst);
     BonusInst.setName(NewBonusInst->getName() + ".old");
 
     // Update (liveout) uses of bonus instructions,
     // now that the bonus instruction has been cloned into predecessor.
     SSAUpdater SSAUpdate;
if (SpeculatedStoreValue) {
     SpeculatedStore->applyMergedLocation(BI->getDebugLoc(),
                                          SpeculatedStore->getDebugLoc());
   }
 
   // Metadata can be dependent on the condition we are hoisting above.
   // Conservatively strip all metadata on the instruction. Drop the debug loc
   // to avoid making it appear as if the condition is a constant, which would
   // be misleading while debugging.
+  // Similarly strip attributes that maybe dependent on condition we are
+  // hoisting above.
   for (auto &I : *ThenBB) {
     if (!SpeculatedStoreValue || &I != SpeculatedStore)
       I.setDebugLoc(DebugLoc());
-    I.dropUnknownNonDebugMetadata();
+    I.dropUndefImplyingAttrsAndUnknownMetadata();
   }
 
   // Hoist the instructions.
   BB->getInstList().splice(BI->getIterator(), ThenBB->getInstList(),
                            ThenBB->begin(), std::prev(ThenBB->end()));
 
   // Insert selects and rewrite the PHI operands.
   IRBuilder<NoFolder> Builder(BI);

llvm/test/Transforms/LICM/call-hoisting.ll

loop:
   %iv.next = add i32 %iv, 1
   %cmp = icmp slt i32 %iv, 200
   br i1 %cmp, label %loop, label %exit
 
 exit:
   ret void
 }
 
-declare i32 @spec(i32* %p) readonly argmemonly nounwind speculatable
+declare i32 @spec(i32* %p, i32* %q) readonly argmemonly nounwind speculatable
 
-; FIXME: We should strip the nonnull callsite attribute on spec call's argument since it is
-; may not be valid when hoisted to preheader.
+; We should strip the dereferenceable callsite attribute on spec call's argument since it is
+; can cause UB in the speculatable call when hoisted to preheader.
+; However, we need not strip the nonnull attribute since it just propagates
+; poison if the parameter was indeed nonnull.
 define void @test_strip_attribute(i32* noalias %loc, i32* noalias %sink, i32* %q) {
-; CHECK-LABEL: test_strip_attribute
-; CHECK-LABEL: entry
-; CHECK-NEXT:   %ret = call i32 @load(i32* %loc)
-; CHECK-NEXT:   %nullchk = icmp eq i32* %q, null
-; CHECK-NEXT:   %ret2 = call i32 @spec(i32* nonnull %q)
+; CHECK-LABEL: @test_strip_attribute(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[RET:%.*]] = call i32 @load(i32* [[LOC:%.*]])
+; CHECK-NEXT:    [[NULLCHK:%.*]] = icmp eq i32* [[Q:%.*]], null
+; CHECK-NEXT:    [[RET2:%.*]] = call i32 @spec(i32* [[Q]], i32* [[LOC]])
+; CHECK-NEXT:    br label [[LOOP:%.*]]
+; CHECK:       loop:
+; CHECK-NEXT:    [[IV:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[ISNULL:%.*]] ]
+; CHECK-NEXT:    br i1 [[NULLCHK]], label [[ISNULL]], label [[NONNULLBB:%.*]]
 entry:
   br label %loop
 
 loop:
   %iv = phi i32 [0, %entry], [%iv.next, %isnull ]
   %ret = call i32 @load(i32* %loc)
   %nullchk = icmp eq i32* %q, null
   br i1 %nullchk, label %isnull, label %nonnullbb
 
-nonnullbb:  
-  %ret2 = call i32 @spec(i32* nonnull %q)
+nonnullbb:
+  %ret2 = call i32 @spec(i32* nonnull %q, i32* dereferenceable(12) %loc)
   br label %isnull
 
-isnull:  
+isnull:
   store volatile i32 %ret, i32* %sink
   %iv.next = add i32 %iv, 1
   %cmp = icmp slt i32 %iv, 200
   br i1 %cmp, label %loop, label %exit
 
 exit:
   ret void
 }

 entry:
   br label %loop
 
 loop:
   %iv = phi i32 [0, %entry], [%iv.next, %backedge]
   call void @store(i32 0, i32* %loc)
   %iv.next = add i32 %iv, 1
   br i1 %earlycnd, label %exit1, label %backedge
-  
 backedge:
   %cmp = icmp slt i32 %iv, 200
   br i1 %cmp, label %loop, label %exit2
 
 exit1:
   ret void
 exit2:
   ret void
entry:
   br label %loop
 
 loop:
   %iv = phi i32 [0, %entry], [%iv.next, %backedge]
   call void @store(i32 0, i32* %loc)
   %v = load i32, i32* %loc
   %earlycnd = icmp eq i32 %v, 198
   br i1 %earlycnd, label %exit1, label %backedge
-  
 backedge:
   %iv.next = add i32 %iv, 1
   %cmp = icmp slt i32 %iv, 200
   br i1 %cmp, label %loop, label %exit2
 
 exit1:
   ret void
 exit2:

llvm/test/Transforms/SimplifyCFG/fold-branch-to-common-dest.ll

 final_left:
   call void @sideeffect0()
   ret void
 final_right:
   call void @sideeffect1()
   ret void
 }
 
-; FIXME: When we fold the dispatch block into pred, the call is moved to pred
-; and the attribute nonnull is no longer valid on paramater. We should drop it.
+; When we fold the dispatch block into pred, the call is moved to pred
+; and the attribute nonnull propagates poison paramater. However, since the
+; function is speculatable, it can never cause UB. So, we need not technically drop it.
 define void @one_pred_with_spec_call(i8 %v0, i8 %v1, i32* %p) {
 ; CHECK-LABEL: one_pred_with_spec_call
 ; CHECK-LABEL: pred:
 ; CHECK:         %c0 = icmp ne i32* %p, null
-; CHECK:         %x = call i32 @speculate_call(i32* nonnull %p)
+; CHECK:         %x = call i32 @speculate_call(i32* %p)
 pred:
   %c0 = icmp ne i32* %p, null
   br i1 %c0, label %dispatch, label %final_right
 
 dispatch:
   %x = call i32 @speculate_call(i32* nonnull %p)
   %c1 = icmp eq i8 %v1, 0
   br i1 %c1, label %final_left, label %final_right
 
 final_left:
   ret void
 
 final_right:
   call void @sideeffect0()
   ret void
 }
 
+; Drop dereferenceable on the parameter
+define void @one_pred_with_spec_call_deref(i8 %v0, i8 %v1, i32* %p) {
+; CHECK-LABEL: one_pred_with_spec_call_deref
+; CHECK-LABEL: pred:
+; CHECK:         %c0 = icmp ne i32* %p, null
+; CHECK:         %x = call i32 @speculate_call(i32* %p)
+pred:
+  %c0 = icmp ne i32* %p, null
+  br i1 %c0, label %dispatch, label %final_right
+
+dispatch:
+  %x = call i32 @speculate_call(i32* dereferenceable(12) %p)
+  %c1 = icmp eq i8 %v1, 0
+  br i1 %c1, label %final_left, label %final_right
+
+final_left:
+  ret void
+
+final_right:
+  call void @sideeffect0()
+  ret void
+}
+
 define void @two_preds_with_extra_op(i8 %v0, i8 %v1, i8 %v2, i8 %v3) {
 ; CHECK-LABEL: @two_preds_with_extra_op(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[C0:%.*]] = icmp eq i8 [[V0:%.*]], 0
 ; CHECK-NEXT:    br i1 [[C0]], label [[PRED0:%.*]], label [[PRED1:%.*]]
 ; CHECK:       pred0:
 ; CHECK-NEXT:    [[C1:%.*]] = icmp eq i8 [[V1:%.*]], 0
 ; CHECK-NEXT:    [[V3_ADJ_OLD:%.*]] = add i8 [[V1]], [[V2:%.*]]

llvm/test/Transforms/SimplifyCFG/speculate-call.ll

end:
   %ret = phi i32 [%val, %if], [0, %entry]
   ret i32 %ret
 }
 
 define i32 @func() #0 {
   ret i32 1
 }
 
-; FIXME: We should correctly drop the attribute since it may no longer be valid
+; We should correctly drop the attribute since it may no longer be valid
 ; in the context the call is moved to.
+; Since the function is speculatable, the nonnull attribute need not be dropped
+; since it propagates poison (and call executes fine) if the parameter is indeed
+; null.
 define i32 @strip_attr(i32 * %p) {
 ; CHECK-LABEL: strip_attr  
 ; CHECK-LABEL: entry:
 ; CHECK:         %nullchk = icmp ne i32* %p, null
-; CHECK:         %val = call i32 @func_deref(i32* nonnull %p)
+; CHECK:         %val = call i32 @func_nonnull(i32* %p)
 ; CHECK:         select 
 entry:
   %nullchk = icmp ne i32* %p, null
   br i1 %nullchk, label %if, label %end
 
 if:
-  %val = call i32 @func_deref(i32* nonnull %p) #1
+  %val = call i32 @func_nonnull(i32* nonnull %p) #1
   br label %end
 
 end:
   %ret = phi i32 [%val, %if], [0, %entry]
   ret i32 %ret
 }
 
-declare i32 @func_deref(i32*) #1
+; We should strip the deref attribute since it can cause UB when the
+; speculatable call is moved.
+define i32 @strip_attr2(i32 * %p) {
+; CHECK-LABEL: strip_attr2  
+; CHECK-LABEL: entry:
+; CHECK:         %nullchk = icmp ne i32* %p, null
+; CHECK:         %val = call i32 @func_nonnull(i32* %p)
+; CHECK:         select 
+entry:
+  %nullchk = icmp ne i32* %p, null
+  br i1 %nullchk, label %if, label %end
+
+if:
+  %val = call i32 @func_nonnull(i32* dereferenceable(12) %p) #1
+  br label %end
+
+end:
+  %ret = phi i32 [%val, %if], [0, %entry]
+  ret i32 %ret
+}
+
+declare i32 @func_nonnull(i32*) #1
 
 attributes #0 = { nounwind readnone speculatable }
 attributes #1 = { nounwind argmemonly speculatable }