On Wed, May 22, 2024 at 09:46:41AM +0200, Richard Biener wrote:
> On Wed, May 22, 2024 at 3:58 AM liuhongt <hongtao....@intel.com> wrote:
> >
> > According to IEEE standard, for conversions from floating point to
> > integer. When a NaN or infinite operand cannot be represented in the
> > destination format and this cannot otherwise be indicated, the invalid
> > operation exception shall be signaled. When a numeric operand would
> > convert to an integer outside the range of the destination format, the
> > invalid operation exception shall be signaled if this situation cannot
> > otherwise be indicated.
> >
> > The patch prevent simplication of the conversion from floating point
> > to integer for NAN/INF/out-of-range constant when flag_trapping_math.
> >
> > Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}
> > Ok for trunk?
>
> OK if there are no further comments today.
As I wrote in the PR, I don't think this is the right fix for the PR,
the simplify-rtx.cc change is the right thing to do, the C standard
in F.4 says that the out of range conversions to integers should raise
exceptions, but still says that the resulting value in those cases is
unspecified.
So, for the C part we should verify that with -ftrapping-math we don't
constant fold it and cover it both by pure C and perhaps backend specific
testcases which just search asm for the conversion instructions
or even runtime test which tests that the exceptions are triggered,
verify that we don't fold it either during GIMPLE opts or RTL opts
(dunno whether they can be folded in e.g. C constant initializers or not).
And then on the backend side, it should stop using FIX/UNSIGNED_FIX RTLs
in patterns which are used for the intrinsics (and keep using them in
patterns used for C scalar/vector conversions), because even with
-fno-trapping-math the intrinsics should have the documented behavior,
particular result value, while in C they are clearly unspecified and
FIX/UNSIGNED_FIX folding even with the patch chooses some particular values
which don't match those (sure, they are like that because of Java, but am
not sure it is the right time to change what we do in those cases say
by providing a target hook to pick a different value).
The provided testcase tests the values though, so I think is inappropriate
for this patch.
Jakub
