[Verifier] Additional check for get.active.lane.mask

[SjoerdMeijer]

Summary

Adapt the verifier checks for get.active.lane.mask to the new semantics as proposed in D86147.
I.e., we expect second argument %n to be greater than 0, so check that.

[samparker]

llvm/test/Verifier/get-active-lane-mask.ll

24

Tests for (i32 1) and (i32 -1) as well?

[SjoerdMeijer]

Yep, thanks, now with extra tests for (i32 1) and (i32 -1).

[samparker]

llvm/test/Verifier/get-active-lane-mask.ll

36

I'm not familiar with verifying tests.. but why isn't this checking for the same result as the zero input case?

[SjoerdMeijer]

llvm/test/Verifier/get-active-lane-mask.ll

36

because we're checking %TC > 0 here, so 0 should throw a verifier message, and all the other values should be accepted.

[samparker]

llvm/test/Verifier/get-active-lane-mask.ll

36

So.... shouldn't -1 also throw the same error?

[SjoerdMeijer]

llvm/test/Verifier/get-active-lane-mask.ll

36

Ah, sorry, this comment is just about the -1 case, I didn't read properly and thought it was about the 1 and -1 cases.

The -1 is interpreted as an unsigned value (so is a big positive number). I think that's what we want because the semantics of the intrinsics is an icmp ult.

[samparker]

llvm/test/Verifier/get-active-lane-mask.ll

36

Okay, seems a little counter-intuitive after reading the assertion code, it looks like we could just check against a zero value instead of doing zext(TC) > 0.

[SjoerdMeijer]

Cheers, that's indeed a bit shorter.

[samparker]

Thanks, LGTM

[efriedma]

This check isn't appropriate. Given that the second argument to @llvm.get.active.lane.mask.v4i1.i32 is a variable, in general, it isn't appropriate to print an error in cases where we can prove the value is 0. You can add a Lint check if you want.

Also, if @llvm.get.active.lane.mask.v4i1.i32(i32 0, i32 0) is supposed to produce poison, that isn't documented at all.

[SjoerdMeijer]

In D86301#2237384, @efriedma wrote:

This check isn't appropriate. Given that the second argument to @llvm.get.active.lane.mask.v4i1.i32 is a variable, in general, it isn't appropriate to print an error in cases where we can prove the value is 0. You can add a Lint check if you want.

Thanks for pointing this out, I would not have guessed that we shouldn't verify the cases that we can verify.... Just for my understanding, before I revert this, can you explain this? I do see of course that variables and constants get a different treatment here. Is it this discrepancy that makes this not appropriate?

Also, if @llvm.get.active.lane.mask.v4i1.i32(i32 0, i32 0) is supposed to produce poison, that isn't documented at all.

We have added that %n > 0 to the definition, but it sounds like I need to add " and it produces poison values otherwise". I will create a diff for that.

[efriedma]

The problem with checking for 0 is that it makes a bunch of transforms much harder. For example, if you have "add i32 0, 0", we want to fold that to "i32 0". With this verifier check, every pass that does this sort of transform would need to check if the value is used by a llvm.get.active.lane.mask() call.

The Lint pass exists to find code that is technically valid, but not sane.

[SjoerdMeijer]

Ah yes, thanks Eli!
This is reverted in rG1d8af682ef1d, and I will move this to Lint.

llvm/lib/IR/Verifier.cpp

case Intrinsic::eh_exceptionpointer: {
     break;
   }
   case Intrinsic::get_active_lane_mask: {
     Assert(Call.getType()->isVectorTy(), "get_active_lane_mask: must return a "
            "vector", Call);
     auto *ElemTy = Call.getType()->getScalarType();
     Assert(ElemTy->isIntegerTy(1), "get_active_lane_mask: element type is not "
            "i1", Call);
+    if (auto *TripCount = dyn_cast<ConstantInt>(Call.getArgOperand(1)))
+      Assert(!TripCount->isZero(), "get_active_lane_mask: operand #2 "
+             "must be greater than 0", Call);
     break;
   }
   case Intrinsic::masked_load: {
     Assert(Call.getType()->isVectorTy(), "masked_load: must return a vector",
            Call);
 
     Value *Ptr = Call.getArgOperand(0);
     ConstantInt *Alignment = cast<ConstantInt>(Call.getArgOperand(1));

llvm/test/Verifier/get-active-lane-mask.ll

 ; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
 
-declare <4 x i32> @llvm.get.active.lane.mask.v4i32.i32(i32, i32)
 
-define <4 x i32> @t1(i32 %IV, i32 %BTC) {
+define <4 x i32> @t1(i32 %IV, i32 %TC) {
 ; CHECK:      get_active_lane_mask: element type is not i1
-; CHECK-NEXT: %res = call <4 x i32> @llvm.get.active.lane.mask.v4i32.i32(i32 %IV, i32 %BTC)
+; CHECK-NEXT: %res = call <4 x i32> @llvm.get.active.lane.mask.v4i32.i32(i32 %IV, i32 %TC)
 
-  %res = call <4 x i32> @llvm.get.active.lane.mask.v4i32.i32(i32 %IV, i32 %BTC)
+  %res = call <4 x i32> @llvm.get.active.lane.mask.v4i32.i32(i32 %IV, i32 %TC)
   ret <4 x i32> %res
 }
 
-declare i32 @llvm.get.active.lane.mask.i32.i32(i32, i32)
-
-define i32 @t2(i32 %IV, i32 %BTC) {
+define i32 @t2(i32 %IV, i32 %TC) {
 ; CHECK:      Intrinsic has incorrect return type!
 ; CHECK-NEXT: i32 (i32, i32)* @llvm.get.active.lane.mask.i32.i32
 
-  %res = call i32 @llvm.get.active.lane.mask.i32.i32(i32 %IV, i32 %BTC)
+  %res = call i32 @llvm.get.active.lane.mask.i32.i32(i32 %IV, i32 %TC)
   ret i32 %res
 }
+
+define <4 x i1> @t3(i32 %IV) {
+; CHECK:      get_active_lane_mask: operand #2 must be greater than 0
+; CHECK-NEXT: %res = call <4 x i1> @llvm.get.active.lane.mask.v4i1.i32(i32 %IV, i32 0)
+
+  %res = call <4 x i1> @llvm.get.active.lane.mask.v4i1.i32(i32 %IV, i32 0)
+  ret <4 x i1> %res
+}
+
+define <4 x i1> @t4(i32 %IV) {
+; CHECK-NOT: get_active_lane_mask
+; CHECK-NOT: call <4 x i1> @llvm.get.active.lane.mask
+
+  %res = call <4 x i1> @llvm.get.active.lane.mask.v4i1.i32(i32 %IV, i32 1)
+  ret <4 x i1> %res
+}
+
+define <4 x i1> @t5(i32 %IV) {
+; CHECK-NOT: get_active_lane_mask
+; CHECK-NOT: call <4 x i1> @llvm.get.active.lane.mask
+
+  %res = call <4 x i1> @llvm.get.active.lane.mask.v4i1.i32(i32 %IV, i32 -1)
+  ret <4 x i1> %res
+}
+
+declare <4 x i32> @llvm.get.active.lane.mask.v4i32.i32(i32, i32)
+declare i32 @llvm.get.active.lane.mask.i32.i32(i32, i32)
+declare <4 x i1> @llvm.get.active.lane.mask.v4i1.i32(i32, i32)