[Verifier] Always verify all assume bundles.

[fhahn]

For some reason, some of the checks for specific assumbe bundle elements exit early if the check pass, meaning we don't verify other entries. Replace the early returns with early continues.

This also requires removing some tests that are currently rejected.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Florian Hahn (fhahn)

Changes

For some reason, some of the checks for specific assumbe bundle elements exit early if the check pass, meaning we don't verify other entries.

This let a few cases with deref assumptions and non-constant integers slip through. The patch also adds dedicated checks for assume bundles, allowing non-constant integers. This is similar to how align is handled.

We could also first just replace the early returns with continues, but that would require adjusting some tests.

---

Full diff: https://github.com/llvm/llvm-project/pull/145586.diff

2 Files Affected:

• (modified) llvm/lib/IR/Verifier.cpp (+11-2)
• (modified) llvm/test/Verifier/assume-bundles.ll (+5-6)

diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 71261343b3482..15bf9d9201070 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -5517,7 +5517,7 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
                   Call.getOperand(Elem.Begin + 1)->getType()->isPointerTy(),
               "arguments to separate_storage assumptions should be pointers",
               Call);
-        return;
+        continue;
       }
       Check(Elem.Tag->getKey() == "ignore" ||
                 Attribute::isExistingAttribute(Elem.Tag->getKey()),
@@ -5534,7 +5534,16 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
         if (ArgCount == 3)
           Check(Call.getOperand(Elem.Begin + 2)->getType()->isIntegerTy(),
                 "third argument should be an integer if present", Call);
-        return;
+        continue;
+      }
+      if (Kind == Attribute::Dereferenceable) {
+        Check(ArgCount == 2,
+              "dereferenceable assumptions should have 2 arguments", Call);
+        Check(Call.getOperand(Elem.Begin)->getType()->isPointerTy(),
+              "first argument should be a pointer", Call);
+        Check(Call.getOperand(Elem.Begin + 1)->getType()->isIntegerTy(),
+              "second argument should be an integer", Call);
+        continue;
       }
       Check(ArgCount <= 2, "too many arguments", Call);
       if (Kind == Attribute::None)
diff --git a/llvm/test/Verifier/assume-bundles.ll b/llvm/test/Verifier/assume-bundles.ll
index 4b6971d6be832..26f5422444d0b 100644
--- a/llvm/test/Verifier/assume-bundles.ll
+++ b/llvm/test/Verifier/assume-bundles.ll
@@ -7,13 +7,13 @@ define void @func(ptr %P, i32 %P1, ptr %P2, ptr %P3) {
 ; CHECK: tags must be valid attribute names
 ; CHECK: "adazdazd"
   call void @llvm.assume(i1 true) ["adazdazd"()]
-; CHECK: the second argument should be a constant integral value
+; CHECK-NOT: call {{.+}}dereferenceable
   call void @llvm.assume(i1 true) ["dereferenceable"(ptr %P, i32 %P1)]
-; CHECK: the second argument should be a constant integral value
+; CHECK: second argument should be an integer
   call void @llvm.assume(i1 true) ["dereferenceable"(ptr %P, float 1.5)]
-; CHECK: too many arguments
+; CHECK: dereferenceable assumptions should have 2 arguments
   call void @llvm.assume(i1 true) ["dereferenceable"(ptr %P, i32 8, i32 8)]
-; CHECK: this attribute should have 2 arguments
+; CHECK: dereferenceable assumptions should have 2 arguments
   call void @llvm.assume(i1 true) ["dereferenceable"(ptr %P)]
 ; CHECK: this attribute has no argument
   call void @llvm.assume(i1 true) ["dereferenceable"(ptr %P, i32 4), "cold"(ptr %P)]
@@ -30,8 +30,7 @@ define void @func(ptr %P, i32 %P1, ptr %P2, ptr %P3) {
   call void @llvm.assume(i1 true) ["separate_storage"(ptr %P)]
 ; CHECK: arguments to separate_storage assumptions should be pointers
   call void @llvm.assume(i1 true) ["separate_storage"(ptr %P, i32 123)]
-; FIXME: The dereferenceable bundle is invalid.
-; CHECK-NOT: call {{.+}}dereferenceable
+; CHECK: dereferenceable assumptions should have 2 arguments
   call void @llvm.assume(i1 true) ["align"(ptr %P, i32 4), "dereferenceable"(ptr %P)]
   ret void
 }

[nikic]

It wasn't really clear to me from the PR description, but the core functional change here is that dereferenceable bundles now allow non-constant sizes?

[efriedma-quic]

My reading is that some regression tests accidentally have dereferenceable bundles with non-constant sizes, and nobody noticed it wasn't actually supposed to be valid.

Assuming it's only a couple tests, I think I'd prefer to fix/delete the tests, then add new tests in #128436... although it's probably not a big deal either way.

[fhahn]

My reading is that some regression tests accidentally have dereferenceable bundles with non-constant sizes, and nobody noticed it wasn't actually supposed to be valid.

Assuming it's only a couple tests, I think I'd prefer to fix/delete the tests, then add new tests in #128436... although it's probably not a big deal either way.

Sonuds good, updated the test to just replace the early returns with early continues, and remove the problematic tests.

[efriedma-quic]

LGTM