[Bug tree-optimization/115427] New: fallback for interclass mathfn bifs like isinf, isfinite, isnormal

[Bug tree-optimization/115427] New: fallback for interclass mathfn bifs like isinf, isfinite, isnormal

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427

            Bug ID: 115427
           Summary: fallback for interclass mathfn bifs like isinf,
                    isfinite, isnormal
           Product: gcc
           Version: 15.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: linkw at gcc dot gnu.org
  Target Milestone: ---

This is filed as follow up for the discussion in [1].

The optabs for isfinite and isnormal would be landed soon, the documentation
allows the optab expansion to fail (as it doesn't mention it's not allowed to),
but with an artificial FAIL in the define_expand for these optabs, there are
two cases:
  1) for isinf, it would result in a call to isinf, but in fact
fold_builtin_interclass_mathfn is able to fold them if there is no target
specific implementation.
  2) for isfinite and isnormal, since there is no library call registered, it
would result in a call to __builtin_{isfinite, isnormal}, which is completely
wrong.

So following Richi's suggestion, this PR is to follow up the falling back way.

[1]
https://inbox.sourceware.org/gcc-patches/17c9ab5d-f1d4-9447-fccf-d9aa0ad561de@linux.ibm.com/

[Bug tree-optimization/115427] fallback for interclass mathfn bifs like isinf, isfinite, isnormal

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |linkw at gcc dot gnu.org
           Keywords|                            |internal-improvement
                 CC|                            |bergner at gcc dot gnu.org,
                   |                            |guihaoc at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org,
                   |                            |rsandifo at gcc dot gnu.org,
                   |                            |segher at gcc dot gnu.org

--- Comment #1 from Kewen Lin <linkw at gcc dot gnu.org> ---
Now we have expand_builtin_interclass_mathfn to expand these functions if they
don't have optab defined, it seems fine to generate equivalent RTL as
fold_builtin_interclass_mathfn there. However, by considering the
maintainability, IMHO it's better to reuse the tree exp in
fold_builtin_interclass_mathfn, then we only have one place for such folding.
It would be like something:

@@ -2534,6 +2536,20 @@ expand_builtin_interclass_mathfn (tree exp, rtx target)
           && maybe_emit_unop_insn (icode, ops[0].value, op0, UNKNOWN))
         return ops[0].value;

+      location_t loc = EXPR_LOCATION (exp);
+      tree fold_res
+        = fold_builtin_interclass_mathfn (loc, fndecl, orig_arg, false);
+
+      if (fold_res)
+        {
+          op0 = expand_expr (fold_res, NULL_RTX, VOIDmode, EXPAND_NORMAL);
+          tree rtype = TREE_TYPE (TREE_TYPE (fndecl));
+          machine_mode rmode = TYPE_MODE (rtype);
+          if (rmode != GET_MODE (op0))
+            op0 = convert_to_mode (rmode, op0, 0);
+          return op0;
+        }
+
       delete_insns_since (last);
       CALL_EXPR_ARG (exp, 0) = orig_arg;

But unfortunately since fold_builtin_interclass_mathfn is for both front-end
and middle-end, it would have some tree code like TRUTH_NOT_EXPR, which isn't
supported in expand_expr. To make it work, we can change TRUTH_NOT_EXPR with
BIT_NOT_EXPR (like in fold_builtin_unordered_cmp), but there are some other
codes like TRUTH_ANDIF_EXPR, TRUTH_ORIF_EXPR (for ibmlongdouble) which can't be
replaced with BIT_AND_EXPR and BIT_OR_EXPR by considering the short-circuit, so
I tried to use COND_EXPR for them instead, but by testing a case with ibmlong
double, there are still some gaps from the original folding code.

I also tried a hackish way that is to force tree exp to gimple stmts and try to
expand these stmts one by one, but it adds more ssa than before and ICE on ssa
to rtx things, not sure if it's a considerable direction to dig into.

I'm looking for suggestions here, is there some existing practice to follow?
which is preferred that expanding from folded tree exp or generating equivalent
rtx directly.  If for the former one, allowing some difference from the
original folding (FAIL can be rare), or experimenting some other ways.

[Bug tree-optimization/115427] fallback for interclass mathfn bifs like isinf, isfinite, isnormal

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
The canonical way would be to handle these in the ISEL pass and remove
the (fallback) expansion.  But then we can see whether the expander FAILs
(ideally expanders would never be allowed to FAIL, and for FAILing expanders
we'd have a way to query the target like we have the vec_perm_const hook).

But I'll note that currently the expanders may FAIL but then we expand to
a call rather than the inline-expansion (and for example AVR relies on this
now to avoid early folding of isnan).

So - for the cases of isfininte and friends without a fallback call I
would suggest to expand from ISEL to see if it FAILs and throw away
the result (similar as how IVOPTs probes things).  Or make those _not_
allowed to FAIL?  Why would they fail to expand anyway?

[Bug tree-optimization/115427] fallback for interclass mathfn bifs like isinf, isfinite, isnormal

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427

--- Comment #3 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #2)
> The canonical way would be to handle these in the ISEL pass and remove
> the (fallback) expansion.  But then we can see whether the expander FAILs
> (ideally expanders would never be allowed to FAIL, and for FAILing expanders
> we'd have a way to query the target like we have the vec_perm_const hook).
> 
> But I'll note that currently the expanders may FAIL but then we expand to
> a call rather than the inline-expansion (and for example AVR relies on this
> now to avoid early folding of isnan).
> 
> So - for the cases of isfininte and friends without a fallback call I
> would suggest to expand from ISEL to see if it FAILs and throw away
> the result (similar as how IVOPTs probes things).  Or make those _not_
> allowed to FAIL?  Why would they fail to expand anyway?

Thanks for the suggestion! IIUC considering the AVR example we still want
*isinf* to fall back with the library call (so not falling back with
inline-expansion way then).  Currently at least for rs6000 port there is no
case that we want to make it FAIL, but not sure some other targets will have
such need in future.  From the review comment[1], we don't note it's not
allowed to FAIL so we probably need to ensure there is some handling for FAIL
in case some future FAIL cause some unexpected failure. Do you prefer not
allowing it to FAIL? then re-open this and go with ISEL if some port wants it
to FAIL?

[1]
https://inbox.sourceware.org/gcc-patches/CAFiYyc3wE=xDkrZuvf1KTtdRKVaaW-dYW+ZtRYC1P6+6NMTZ2Q@mail.gmail.com/

[Bug tree-optimization/115427] fallback for interclass mathfn bifs like isinf, isfinite, isnormal

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427

--- Comment #4 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 11 Jun 2024, linkw at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427
> 
> --- Comment #3 from Kewen Lin <linkw at gcc dot gnu.org> ---
> (In reply to Richard Biener from comment #2)
> > The canonical way would be to handle these in the ISEL pass and remove
> > the (fallback) expansion.  But then we can see whether the expander FAILs
> > (ideally expanders would never be allowed to FAIL, and for FAILing expanders
> > we'd have a way to query the target like we have the vec_perm_const hook).
> > 
> > But I'll note that currently the expanders may FAIL but then we expand to
> > a call rather than the inline-expansion (and for example AVR relies on this
> > now to avoid early folding of isnan).
> > 
> > So - for the cases of isfininte and friends without a fallback call I
> > would suggest to expand from ISEL to see if it FAILs and throw away
> > the result (similar as how IVOPTs probes things).  Or make those _not_
> > allowed to FAIL?  Why would they fail to expand anyway?
> 
> Thanks for the suggestion! IIUC considering the AVR example we still want
> *isinf* to fall back with the library call (so not falling back with
> inline-expansion way then).  Currently at least for rs6000 port there is no
> case that we want to make it FAIL, but not sure some other targets will have
> such need in future.  From the review comment[1], we don't note it's not
> allowed to FAIL so we probably need to ensure there is some handling for FAIL
> in case some future FAIL cause some unexpected failure. Do you prefer not
> allowing it to FAIL? then re-open this and go with ISEL if some port wants it
> to FAIL?

I think it would be cleaner to not allow it FAIL since there's no library
fallback.  FAILing patterns are a hassle when it comes to GIMPLE
optimizations.

As said, there should be a good reason why patterns FAIL - what's
the idea behind this feature anyway?

[Bug tree-optimization/115427] fallback for interclass mathfn bifs like isinf, isfinite, isnormal

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427

--- Comment #5 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #4)
> On Tue, 11 Jun 2024, linkw at gcc dot gnu.org wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427
> > 
> > --- Comment #3 from Kewen Lin <linkw at gcc dot gnu.org> ---
> > (In reply to Richard Biener from comment #2)
> > > The canonical way would be to handle these in the ISEL pass and remove
> > > the (fallback) expansion.  But then we can see whether the expander FAILs
> > > (ideally expanders would never be allowed to FAIL, and for FAILing expanders
> > > we'd have a way to query the target like we have the vec_perm_const hook).
> > > 
> > > But I'll note that currently the expanders may FAIL but then we expand to
> > > a call rather than the inline-expansion (and for example AVR relies on this
> > > now to avoid early folding of isnan).
> > > 
> > > So - for the cases of isfininte and friends without a fallback call I
> > > would suggest to expand from ISEL to see if it FAILs and throw away
> > > the result (similar as how IVOPTs probes things).  Or make those _not_
> > > allowed to FAIL?  Why would they fail to expand anyway?
> > 
> > Thanks for the suggestion! IIUC considering the AVR example we still want
> > *isinf* to fall back with the library call (so not falling back with
> > inline-expansion way then).  Currently at least for rs6000 port there is no
> > case that we want to make it FAIL, but not sure some other targets will have
> > such need in future.  From the review comment[1], we don't note it's not
> > allowed to FAIL so we probably need to ensure there is some handling for FAIL
> > in case some future FAIL cause some unexpected failure. Do you prefer not
> > allowing it to FAIL? then re-open this and go with ISEL if some port wants it
> > to FAIL?
> 
> I think it would be cleaner to not allow it FAIL since there's no library
> fallback.  

Fair enough!

> FAILing patterns are a hassle when it comes to GIMPLE
> optimizations.

Yeah, for some cases port isn't able to put some condition as part of condition
HAVE_* (such as further checking operand special values etc.), FAIL has to be
used.

> 
> As said, there should be a good reason why patterns FAIL - what's
> the idea behind this feature anyway?

No solid input for this, as the proposed documentation implicitly indicates
FAIL is possible to be used (like some other existing expanders), I didn't
consider carefully if it has a good reason, but just assuming it can happen. :(
It's a really good question if there will be a need for it.