[PATCH] D54649: [FPEnv] Rough out constrained FCmp intrinsics

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added inline comments.


================
Comment at: include/llvm/IR/Intrinsics.h:104
+      SameVecWidthArgument, PtrToArgument, PtrToElt, VecOfAnyPtrsToElt,
+      SameVecLengthOrScalarArgument
     } Kind;
----------------
cameron.mcinally wrote:
> andrew.w.kaylor wrote:
> > cameron.mcinally wrote:
> > > andrew.w.kaylor wrote:
> > > > cameron.mcinally wrote:
> > > > > andrew.w.kaylor wrote:
> > > > > > This should probably be called SameVectorWidthOrScalarArgument for consistency.
> > > > > Ah, so we actually discussed this in one of the first comments. My view is that `Vector Width` describes the bit-width of the vector. And `Vector Length` describes the number of elements in the vector. If everyone agrees, then it would be correct as VecLength.
> > > > > 
> > > > Does 'length' mean the same thing that 'width' means in SameVecWidthArgument on the previous line? I'm happy with either wording as long as they're consistent.
> > > It does not. E.g. <2 x i32> has a length of 2 and width of 64.
> > Is that what we want? For instance, would you want to be able to compare a <4 x f32> with a <2 x f64>?
> > 
> > What I was expecting was something that would require the vector to have the same element type and the same number of elements.
> Let's look at a hard example:
> 
> ```
> def int_experimental_constrained_fcmpeq : Intrinsic<[ llvm_anybool_ty ],
>   [ LLVMVectorSameLengthOrScalar<0, llvm_anyfloat_ty>,
>     LLVMVectorSameLengthOrScalar<0, llvm_anyfloat_ty>,
>     llvm_metadata_ty,
>     llvm_metadata_ty ]>;
> ```
> 
> We want the vector length to match between the <N x i1> return type and the <N x fp> operands. Both input arguments have the same type.
> 
> Now that I look at it, the 2nd LLVMVectorSameLengthOrScalar could probably just be a:
> 
> LLVMMatchType<1>
Oh, I'm sorry. I read your explanation backward. For some reason I had it in my head that the vector width (128/256 or whatever) was what you were introducing.

Now I understand, and your choice makes perfect sense.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54649