[PATCH] D69891: [VP,Integer,#1] Vector-predicated integer intrinsics

[Simon Moll via Phabricator via llvm-commits]

simoll marked 4 inline comments as done.
simoll added inline comments.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:276
+  auto VLConst = dyn_cast<ConstantInt>(VLParam);
+  if (VLConst && VLConst->getSExtValue() == -1)
+    return true;
----------------
andrew.w.kaylor wrote:
> simoll wrote:
> > andrew.w.kaylor wrote:
> > > Why are you doing this and not just "less than zero"?
> > Because with the new LangRef wording %evl is unsigned and the `i32 -1` constant is handled as a special case.
> I see that you also discussed this with Eli. I don't like the fact that constant -1 and a value that is 0xFFFFFFFF at runtime could have different behaviors. There's no telling when the constant folder might find a path to convert a value to a constant. I understand that the intrsincs aren't supposed to be called with values outside the range of [0, W] and has undefined behavior if it is, but having an exception for a constant does seem right.
> 
> As a completely contrived example, suppose I had IR like this:
> 
> ```
> bb1:
>   br i1 %flag, label %bb2, label %bb3
> 
> bb2:
>   %t1 = call <8 x i32> @llvm.vp.add.v8i32(<8 x i32> %i0, <8 x i32> %i1, <8 x i1> %m, i32 %n)
>   br label %bb4
> 
> bb3:
>   %t2 = call <8 x i32> @llvm.vp.add.v8i32(<8 x i32> %i0, <8 x i32> %i1, <8 x i1> %m, i32 -1)
>   br label %bb4
> 
> bb4:
>   %t3 = phi <8 x i32> [%t1, %bb1], [%t2, %bb2]
>   ...
> ```  
> Now some pass is going to transform that into:
> 
> ```
> bb1:
>   %evl = select i1 %flag, i32 %n, i32 -1
>   %t3 = call <8 x i32> @llvm.vp.add.v8i32(<8 x i32> %i0, <8 x i32> %i1, <8 x i1> %m, i32 %evl)
>   ...
> ```  
> And now the -1 case has undefined behavior.
That's a problem, yes.
We could do away with the `-1` special case entirely. Then, to disable evl it has to be set to the number of vector elements. For a <W x ty> vector that's just the constant `W`, for scalable vector types that would be the appropriate `vscale` const exprssion.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69891/new/

https://reviews.llvm.org/D69891