Strip context sensitive attributes when call is moved

[anna]

already reviewed and landed in another patch.

llvm/include/llvm/IR/Attributes.h

public:
 
   /// Return true if the attribute is a string (target-dependent)
   /// attribute.
   bool isStringAttribute() const;
 
   /// Return true if the attribute is a type attribute.
   bool isTypeAttribute() const;
 
+  /// Return true if the attribute is an ABI attribute.
+  bool isABIRelated() const;
+
   /// Return true if the attribute is any kind of attribute.
   bool isValid() const { return pImpl; }
 
   /// Return true if the attribute is present.
   bool hasAttribute(AttrKind Val) const;
 
   /// Return true if the target-dependent attribute is present.
   bool hasAttribute(StringRef Val) const;

llvm/include/llvm/IR/Instruction.h

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
+  /// attributes that are legal to drop. Both of these should be done by
+  /// passes which moves instructions in IR. 
+  void dropContextSensitiveAttributesAndUnknownMetadata();
+
+  /// Use this API when we need to preserve some metadata through KnownIDs.
+  void
+  dropContextSensitiveAttributesAndUnknownMetadata(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::isStringAttribute() const {
   return pImpl && pImpl->isStringAttribute();
 }
 
 bool Attribute::isTypeAttribute() const {
   return pImpl && pImpl->isTypeAttribute();
 }
 
+bool Attribute::isABIRelated() const {
+  if (!pImpl) return false;
+  Attribute::AttrKind A = pImpl->getKindAsEnum();
+  return A == Attribute::SExt || A == Attribute::ZExt; 
+}
+
 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 {

llvm/lib/IR/Instruction.cpp

void Instruction::dropPoisonGeneratingFlags() {
 
   case Instruction::GetElementPtr:
     cast<GetElementPtrInst>(this)->setIsInBounds(false);
     break;
   }
   // TODO: FastMathFlags!
 }
 
+void Instruction::dropContextSensitiveAttributesAndUnknownMetadata() {
+  dropContextSensitiveAttributesAndUnknownMetadata(None);
+}
+
+void Instruction::dropContextSensitiveAttributesAndUnknownMetadata(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
+  // not ABI related. These attributes may be context sensitive and not valid in
+  // the context this instruction is moved to.
+  AttributeList AL = CB->getAttributes();
+  if (AL.isEmpty())
+    return;
+  
+  for (unsigned ArgNo = 0; ArgNo < CB->getNumArgOperands(); ArgNo++) {
+    auto PAttr = AL.getParamAttributes(ArgNo); 
+    // List of attributes that are present in parameter that we need to remove.
+    AttrBuilder R;
+    for (auto A: PAttr) {
+      if (!A.isABIRelated())
+        R.addAttribute(A);
+    }
+    auto Index = ArgNo + AttributeList::FirstArgIndex;
+    CB->setAttributes(CB->getAttributes().removeAttributes(CB->getContext(), Index, R));
+  }
+}
 
 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 parameter attributes on the call
+  // 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.dropContextSensitiveAttributesAndUnknownMetadata();
 
   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/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 maybe invalid in location we moved to. 
+    NewBonusInst->dropContextSensitiveAttributesAndUnknownMetadata(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.dropContextSensitiveAttributesAndUnknownMetadata();
   }
 
   // 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:
   br i1 %cmp, label %loop, label %exit
 
 exit:
   ret void
 }
 
 declare i32 @spec(i32* %p) readonly argmemonly nounwind speculatable
 
-; FIXME: We should strip the nonnull callsite attribute on spec call's argument since it is
+; We should strip the nonnull callsite attribute on spec call's argument since it is
 ; may not be valid when hoisted to preheader.
 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]])
+; 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:  
+nonnullbb:
   %ret2 = call i32 @spec(i32* nonnull %q)
   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
+; 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.
 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

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.
 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_deref(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
   br label %end