Re: [PATCH v3 1/5] LoongArch: Fix usage of LSX and LASX frint/ftint instructions [PR112578]

[Xi Ruoyao <xry111@xry111.site>]

On Thu, 2023-11-23 at 18:12 +0800, Xi Ruoyao wrote:
> On Thu, 2023-11-23 at 17:12 +0800, chenglulu wrote:
> > 
> > 在 2023/11/23 下午5:02, Xi Ruoyao 写道:
> > > On Thu, 2023-11-23 at 16:13 +0800, chenglulu wrote:
> > > > The fix_truncv4sfv4si2 template is indeed called when debugging with
> > > > gdb.
> > > > 
> > > > So I think we can use define_expand here.
> > > The problem is cases where we want to combine an rint call with float-
> > > to-int conversion:
> > > 
> > > float x[4];
> > > int y[4];
> > > 
> > > void test()
> > > {
> > > 	for (int i = 0; i < 4; i++)
> > > 		y[i] = __builtin_rintf(x[i]);
> > > }
> > > 
> > > With define_expand we get "vfrint + vftintrz", but with define_insn we
> > > get a single "vftint".
> > > 
> > > Arguably the generic code should try to handle this (PR86609), but it's
> > > "not sure if that's a good idea in general" (comment 1 in the PR) so we
> > > can do this in a target-specific way.
> > > 
> > I tried to use Ofast to compile, and found that a vftint was generated, 
> > and at.006t.gimple appeared.
> > 
> > If O2 was compiled, __builtin_rintf would be generated, but Ofast would 
> > generate __builtin_irintf
> 
> Indeed...  It seems the FE will only generate __builtin_irintf when -
> fno-math-errno -funsafe-math-optimizations.
> 
> But I cannot see why this is necessary (at least for us): the rintf
> function does not set errno at all, and to me using vftint.w.s here is
> safe: if the rounded result can be represented as a 32-bit int,
> obviously there is no issue;  otherwise, per C23 section F.4 we should
> raise FE_INVALID and produce unspecified result.  It seems our ftint.w.s
> instruction has the required semantics.
> 
> +Uros and Joseph for some comment about the expected behavior of
> (int)rintf(x).

I've spent some time reading the code and got some results.

For -fno-math-errno, it's for preventing from converting (int)rintf(x)
to a call to the *external* function irintf(x).  The problem is rintf
never sets errno, but irintf may set errno, this was PR 61876.  However
it's not a problem preventing us from using ftint.w.s because this
instruction does not sets errno.

For -funsafe-math-optimizations, there seems a logic error in
convert_to_integer_1:

  /* Convert e.g. (long)round(d) -> lround(d).  */
  /* If we're converting to char, we may encounter differing behavior
     between converting from double->char vs double->long->char.
     We're in "undefined" territory but we prefer to be conservative,
     so only proceed in "unsafe" math mode.  */
  if (optimize
      && (flag_unsafe_math_optimizations
          || (long_integer_type_node
              && outprec >= TYPE_PRECISION (long_integer_type_node))))

But shouldn't we compare against integer_type_node here as we're
handling __builtin_irint etc. of which the output is int (not long) in
this block?

Anyway, both constraints does not apply for our ftint.w.s instruction. 
And IMO the second constraint is a target-independent bug which should
be fixed.  The first constraint must remain there, but it's only for
preventing from mistakenly using an external irint (which may set
errno), not the ftint.w.s instruction (it does not even know errno).  So
we should use the target-specific way, i. e. a define_insn, to ensure
the optimization even if -fmath-errno.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University