[PATCH] D102673: [ConstantFolding] Fold constrained arithmetic intrinsics

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1910-1911
+  RoundingMode RM = RoundingMode::Dynamic;
+  Optional<RoundingMode> ORM = CI->getRoundingMode();
+  if (ORM)
+    RM = *ORM;
----------------
spatel wrote:
> sepavloff wrote:
> > spatel wrote:
> > > I think this would be easier to read if we made it more like the code above here:
> > > 
> > > ```
> > >   Optional<RoundingMode> ORM = CI->getRoundingMode();
> > >   // If no rounding mode is specified by the intrinsic or the mode is dynamic,
> > >   // try to evaluate using the default mode. If it does not raise an inexact
> > >   // exception, rounding was not applied so the result is independent of
> > >   // rounding mode. 
> > >   if (!ORM || *ORM == RoundingMode::Dynamic) 
> > >     return RoundingMode::NearestTiesToEven;
> > >   // Use the mode specified by the intrinsic.
> > >   return *ORM;
> > > ```
> > > That's assuming I'm reading it correctly right now - are there tests with no RM specified on the intrinsic?
> > > That's assuming I'm reading it correctly right now - are there tests with no RM specified on the intrinsic?
> > 
> > No, all affected intrinsics have such argument. Bu in other places such check is made, so I put it here also. 
> I am not understanding something then.
> If the metadata args for rounding mode and exception behavior are required by the LangRef:
> https://llvm.org/docs/LangRef.html#llvm-experimental-constrained-fadd-intrinsic
> ...then why is the result of `getRoundingMode()` or `getExceptionBehavior()` an `Optional` value? If it is not valid IR without those args, we shouldn't be adding compile-time checks for things that can't happen.
> I am not understanding something then.
> If the metadata args for rounding mode and exception behavior are required by the LangRef:
> https://llvm.org/docs/LangRef.html#llvm-experimental-constrained-fadd-intrinsic
> ...then why is the result of `getRoundingMode()` or `getExceptionBehavior()` an `Optional` value? If it is not valid IR without those args, we shouldn't be adding compile-time checks for things that can't happen.

This function processes any `ConstrainedFPIntrinsic`. Some of them do not have rounding mode arguments, like constrained variants of `floor`, `trunc`. `frem` also do not depend on rounding mode, although it has rounding mode argument, probably some day it would be removed.

Using `Optional` as return value of `getRoundingMode()` or `getExceptionBehavior()` is now a part of `ConstrainedFPIntrinsic` interface. It is safer to process results of these function in more general way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102673