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

[Simon Moll via Phabricator via llvm-commits]

simoll 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
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69281