[PATCH] D102673: [ConstantFolding] Fold constrained arithmetic intrinsics
Sanjay Patel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 13 06:40:38 PDT 2021
spatel added inline comments.
================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1907
+ // Even if the rounding mode is unknown, try evaluating the operation.
+ // If it does not raise inexact exception, rounding was not applied
+ // so the result does not depend on rounding mode.
----------------
This code comment does not look accurate.
When we speculatively evaluate the expression, we check that *any* exception was not raised, not just the inexact exception, right?
This raises a question: if we evaluate the expression using NearestTiesToEven, then are we guaranteed that all potential exceptions (even underflow) are identical to any other rounding mode?
================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1910-1911
+ RoundingMode RM = RoundingMode::Dynamic;
+ Optional<RoundingMode> ORM = CI->getRoundingMode();
+ if (ORM)
+ RM = *ORM;
----------------
sepavloff wrote:
> 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.
Ok - thanks for explaining. I didn't think of those other intrinsics. Presumably, we'll add constant folding for those later, so this code will be shared.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102673/new/
https://reviews.llvm.org/D102673
More information about the llvm-commits
mailing list