[PATCH] D69281: [FPEnv][WIP] Constrained FCmp intrinsics

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added inline comments.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:750-791
+  def int_experimental_constrained_fcmpoeq
+      : Intrinsic<[ LLVMScalarOrSameVectorWidth<0, llvm_i1_ty> ],
+                  [ llvm_anyfloat_ty, LLVMMatchType<0>, llvm_metadata_ty ]>;
+  def int_experimental_constrained_fcmpogt
+      : Intrinsic<[ LLVMScalarOrSameVectorWidth<0, llvm_i1_ty> ],
+                  [ llvm_anyfloat_ty, LLVMMatchType<0>, llvm_metadata_ty ]>;
+  def int_experimental_constrained_fcmpoge
----------------
simoll wrote:
> andrew.w.kaylor wrote:
> > cameron.mcinally wrote:
> > > uweigand wrote:
> > > > simoll wrote:
> > > > > Out of curiosity: what is your motivation to have one intrinsic per fcmp predicate instead of, say, an `i8 immarg`? 
> > > > I didn't really make the decision, just took it over from the original patch.  But I agree it seems to make sense simply from a perspective of being explicit in the IR: if there's just a random numeric value, it would make the IR harder to read/write.
> > > It would be hard to read -- unless we wrote an IR decoding routine somewhere. That seems weird though.
> > This is why we used metadata strings for the constraint arguments. I'm still not entirely satisfied with that.  Would it be reasonable to make these arguments constants and add a custom printer and parser for these intrinsics? What about token arguments?
> > This is why we used metadata strings for the constraint arguments. I'm still not entirely satisfied with that. 
> 
> You could invent some `fpenv` operand bundle for fpexcept, fpround, as in:
> 
>     %p = llvm.experimental.constrained.fcmp_ugt(%x, %y) [ "fpenv"(metadata !"fpround.tonearest", metadata !"fpexcept.strict") ]
> 
> Omission would then imply the default fp env (i'd like that for the VP fp intrinsics).
> 
> >> From https://llvm.org/docs/LangRef.html#operand-bundles
> > An operand bundle at a call site cannot change the implementation of the called function. 
> 
> I am not sure what to make out of that statement. I guess it should be ok to also use operand bundles also for the comparison predicate? Otw, it might be possible to change to OpBound specification to allow different implementations.
> 
> Personally, i'd prefer we had a more flexible attribute mechanism, so we could tag-on all of these things:
> 
>     %p = fcmp %x, %y cmp(ugt) fpround(tonearest) fpexcept(strict)
I like the idea of operand bundles for the constraint arguments. They seem naturally separate. I'm less comfortable with the comparison predicate being in the operand bundle because it is essential to the semantic meaning of the operation.

I started an experiment yesterday using token arguments instead of metadata to represent the rounding mode and exception behavior. I can easily shift the tokens to an operand bundle. I think we can use a token argument for the fcmp intrinsic predicate. I'll post an RFC to the mailing list soon to solicit opinions about using token arguments this way.

Regarding more flexible attributes, the problem we were trying to solve with the intrinsics was introducing new semantics in a way that wouldn't break existing optimizations that didn't know about the new semantics. Once these intrinsics are well established and well supported we might be able to merge their behavior back into the instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69281