[AssumeBundles] Enforce constraints on the operand bundle of llvm.assume

[Tyker]

Summary

Add verification that operand bundles on an llvm.assume are well formed to the verify pass.

[Tyker]

rebased

[jdoerfert]

This is cool, thx!

I added two comments below.

llvm/include/llvm/IR/Attributes.h

119

Documentation, please.

[Tyker]

Added documentation

[jdoerfert]

llvm/lib/IR/Verifier.cpp

4326

When we drop uses, should we go to none as a tag so it is actually a valid attribute or do we want to allow a special tag here?

[Tyker]

llvm/lib/IR/Verifier.cpp

4326

in D73404 i named the tag to "ignore" and adapted the verifier to treat is as valid.
but putting a "none" could be another solution if it is a valid attribute.
but it doesn't seem to be considered as a valid by opt and isn't considered valid by isExistingAttribute. it is just the hard coded first value of the enum.

[jdoerfert]

LGTM.

llvm/include/llvm/IR/Attributes.h

public:
 
   static Attribute::AttrKind getAttrKindFromName(StringRef AttrName);
 
   static StringRef getNameFromAttrKind(Attribute::AttrKind AttrKind);
 
   /// Return true if and only if the attribute has an Argument.
   static bool doesAttrKindHaveArgument(Attribute::AttrKind AttrKind);
 
+  /// Return true if the provided string matches the IR name of an attribute.
+  /// example: "noalias" return true but not "NoAlias"
+  static bool isExistingAttribute(StringRef Name);
+
   //===--------------------------------------------------------------------===//
   // Attribute Accessors
   //===--------------------------------------------------------------------===//
 
   /// Return true if the attribute is an Attribute::AttrKind type.
   bool isEnumAttribute() const;
 
   /// Return true if the attribute is an integer attribute.

llvm/lib/IR/Attributes.cpp

 bool Attribute::doesAttrKindHaveArgument(Attribute::AttrKind AttrKind) {
   return AttrKind == Attribute::Alignment ||
          AttrKind == Attribute::StackAlignment ||
          AttrKind == Attribute::Dereferenceable ||
          AttrKind == Attribute::AllocSize ||
          AttrKind == Attribute::DereferenceableOrNull;
 }
 
+bool Attribute::isExistingAttribute(StringRef Name) {
+  return StringSwitch<bool>(Name)
+#define GET_ATTR_NAMES
+#define ATTRIBUTE_ALL(ENUM_NAME, DISPLAY_NAME) .Case(#DISPLAY_NAME, true)
+#include "llvm/IR/Attributes.inc"
+      .Default(false);
+}
+
 //===----------------------------------------------------------------------===//
 // Attribute Accessor Methods
 //===----------------------------------------------------------------------===//
 
 bool Attribute::isEnumAttribute() const {
   return pImpl && pImpl->isEnumAttribute();
 }
 

llvm/lib/IR/KnowledgeRetention.cpp

 
 static Value *getValueFromBundleOpInfo(IntrinsicInst &Assume,
                                 const CallBase::BundleOpInfo &BOI,
                                 unsigned Idx) {
   assert(BundleHasArguement(BOI, Idx) && "index out of range");
   return (Assume.op_begin() + BOI.Begin + Idx)->get();
 }
 
-#ifndef NDEBUG
-
-static bool isExistingAttribute(StringRef Name) {
-  return StringSwitch<bool>(Name)
-#define GET_ATTR_NAMES
-#define ATTRIBUTE_ALL(ENUM_NAME, DISPLAY_NAME) .Case(#DISPLAY_NAME, true)
-#include "llvm/IR/Attributes.inc"
-      .Default(false);
-}
-
-#endif
-
 bool llvm::hasAttributeInAssume(CallInst &AssumeCI, Value *IsOn,
                                 StringRef AttrName, uint64_t *ArgVal,
                                 AssumeQuery AQR) {
   IntrinsicInst &Assume = cast<IntrinsicInst>(AssumeCI);
   assert(Assume.getIntrinsicID() == Intrinsic::assume &&
          "this function is intended to be used on llvm.assume");
-  assert(isExistingAttribute(AttrName) && "this attribute doesn't exist");
+  assert(Attribute::isExistingAttribute(AttrName) &&
+         "this attribute doesn't exist");
   assert((ArgVal == nullptr || Attribute::doesAttrKindHaveArgument(
                                    Attribute::getAttrKindFromName(AttrName))) &&
          "requested value for an attribute that has no argument");
   if (Assume.bundle_op_infos().empty())
     return false;
 
   CallInst::bundle_op_iterator Lookup;
 
   /// The right attribute can be found by binary search. After this finding the
   /// right WasOn needs to be done via linear search.
   /// Element have been ordered by argument value so the first we find is the
   /// one we need.
   if (AQR == AssumeQuery::Lowest)
     Lookup =
         llvm::lower_bound(Assume.bundle_op_infos(), AttrName,
                           [](const CallBase::BundleOpInfo &BOI, StringRef RHS) {
-                            assert(isExistingAttribute(BOI.Tag->getKey()) &&
-                                   "this attribute doesn't exist");
                             return BOI.Tag->getKey() < RHS;
                           });
   else
     Lookup = std::prev(
         llvm::upper_bound(Assume.bundle_op_infos(), AttrName,
                           [](StringRef LHS, const CallBase::BundleOpInfo &BOI) {
-                            assert(isExistingAttribute(BOI.Tag->getKey()) &&
-                                   "this attribute doesn't exist");
                             return LHS < BOI.Tag->getKey();
                           }));
 
   if (Lookup == Assume.bundle_op_info_end() ||
       Lookup->Tag->getKey() != AttrName)
     return false;
   if (IsOn) {
     assert((Lookup->End - Lookup->Begin > BOIE_WasOn) &&

llvm/lib/IR/Verifier.cpp

void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
   // or are local to *this* function.
   for (Value *V : Call.args())
     if (auto *MD = dyn_cast<MetadataAsValue>(V))
       visitMetadataAsValue(*MD, Call.getCaller());
 
   switch (ID) {
   default:
     break;
+  case Intrinsic::assume: {
+    for (auto &Elem : Call.bundle_op_infos()) {
+      Assert(Attribute::isExistingAttribute(Elem.Tag->getKey()),
+             "tags must be valid attribute names");
+      Assert(Elem.End - Elem.Begin <= 2, "to many arguments");
+      Attribute::AttrKind Kind =
+          Attribute::getAttrKindFromName(Elem.Tag->getKey());
+      if (Kind == Attribute::None)
+        break;
+      if (Attribute::doesAttrKindHaveArgument(Kind)) {
+        Assert(Elem.End - Elem.Begin == 2,
+               "this attribute should have 2 arguments");
+        Assert(isa<ConstantInt>(Call.getOperand(Elem.Begin + 1)),
+               "the second argument should be a constant integral value");
+      } else if (isFuncOnlyAttr(Kind)) {
+        Assert((Elem.End - Elem.Begin) == 0, "this attribute has no argument");
+      } else if (!isFuncOrArgAttr(Kind)) {
+        Assert((Elem.End - Elem.Begin) == 1,
+               "this attribute should have one argument");
+      }
+    }
+    break;
+  }
   case Intrinsic::coro_id: {
     auto *InfoArg = Call.getArgOperand(3)->stripPointerCasts();
     if (isa<ConstantPointerNull>(InfoArg))
       break;
     auto *GV = dyn_cast<GlobalVariable>(InfoArg);
     Assert(GV && GV->isConstant() && GV->hasDefinitiveInitializer(),
       "info argument of llvm.coro.begin must refer to an initialized "
       "constant");

llvm/test/IR/assume-builder.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt -passes='assume-builder' -S %s | FileCheck %s --check-prefixes=BASIC
-; RUN: opt -passes='assume-builder' --assume-preserve-all -S %s | FileCheck %s --check-prefixes=ALL
+; RUN: opt -passes='assume-builder,verify' -S %s | FileCheck %s --check-prefixes=BASIC
+; RUN: opt -passes='assume-builder,verify' --assume-preserve-all -S %s | FileCheck %s --check-prefixes=ALL
 
 declare void @func(i32*, i32*)
 declare void @func_cold(i32*) cold
 declare void @func_strbool(i32*) "no-jump-tables"
 declare void @func_many(i32*) "no-jump-tables" nounwind "less-precise-fpmad" willreturn norecurse
 declare void @func_argattr(i32* align 8, i32* nonnull) nounwind
 
 define void @test(i32* %P, i32* %P1, i32* %P2, i32* %P3) {

llvm/test/Verifier/assume-bundles.ll

+; RUN: not opt -verify < %s 2>&1 | FileCheck %s
+
+declare void @llvm.assume(i1)
+
+define void @func(i32* %P, i32 %P1, i32* %P2, i32* %P3) {
+; CHECK: tags must be valid attribute names
+  call void @llvm.assume(i1 true) ["adazdazd"()]
+; CHECK: the second argument should be a constant integral value
+  call void @llvm.assume(i1 true) ["align"(i32* %P, i32 %P1)]
+; CHECK: to many arguments
+  call void @llvm.assume(i1 true) ["align"(i32* %P, i32 8, i32 8)]
+; CHECK: this attribute should have 2 arguments
+  call void @llvm.assume(i1 true) ["align"(i32* %P)]
+; CHECK: this attribute has no argument
+  call void @llvm.assume(i1 true) ["align"(i32* %P, i32 4), "cold"(i32* %P)]
+; CHECK: this attribute should have one argument
+  call void @llvm.assume(i1 true) ["noalias"()]
+  ret void
+}