[Bug tree-optimization/115427] fallback for interclass mathfn bifs like isinf, isfinite, isnormal
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427 --- Comment #5 from Kewen Lin --- (In reply to rguent...@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 --- > > (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.
[Bug tree-optimization/115427] fallback for interclass mathfn bifs like isinf, isfinite, isnormal
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427 --- Comment #4 from 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 --- > (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427 --- Comment #3 from Kewen Lin --- (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+6nmt...@mail.gmail.com/
[Bug tree-optimization/115427] fallback for interclass mathfn bifs like isinf, isfinite, isnormal
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427 --- Comment #2 from Richard Biener --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115427
Kewen Lin 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 ---
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.
