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

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally added inline comments.


================
Comment at: include/llvm/IR/Intrinsics.h:104
+      SameVecWidthArgument, PtrToArgument, PtrToElt, VecOfAnyPtrsToElt,
+      SameVecLengthOrScalarArgument
     } Kind;
----------------
andrew.w.kaylor wrote:
> 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.
No sweat. 

I'll try to address your other comments in the near future. Some higher priority things have come up on my end. Stay tuned...


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54649