On Fri, 28 Aug 2026, Soumya AR wrote: > Hi Richi > > What is your view on where to place the atomic IFN lowering? > > To summarize, in order to avoid extra builtins for atomic fetch min/max ops, > we instead opt to lower generic builtins to an IFN in the frontend, and then > subsequently lower that to CAS loops in the middle-end (if the backend does > not support atomic fetch min/max optabs). > > The IFN-to-CAS lowering happens post-IPA, as with offloading, optab support > can > differ on host / device. > > Therefore, lowering needs to: > > - Run at -O0 / unconditionally > - Run after IPA. > - Have some level of semantic fit with the rest of the pass? > > It would be better to not keep this as a standalone pass, but I'm not sure of > where else would be more suitable for this analysis. > > --- > > Currently, as a proof of concept, I have an experimental patch that shows the > pass can run as part of gimple-isel (which checks the boxes above). > > https://gcc.gnu.org/pipermail/gcc-patches/2026-June/719955.html > > (In hindsight, this could definitely be made more modular to avoid extending > isel too much.) > > But isel runs quite late, and we want to avoid keeping the IFN as a black box > until then. I observed that, if part of an outer loop, we can unroll and > duplicate the IFNs, and lower those to multiple inner CAS loops later (which > did > not happen when lowering early.)
I think ISEL is appropriate. Isn't the unrolling a problem of (size) costing? If you do not expect the CAS loop to be GIMPLE optimzable itself I see no reason to expose it there. You could argue even that IFN RTL expansion is the correct place to deal with this lowering. > --- > > Worth noting that we plan to use pattern-matching to target libstdc++ methods, > where (if we lower too late) we risk a flow where we start with a single > CAS loop, pattern-match to an IFN, and end up duplicating that into several > CAS loops later. See above - the issue is probably that estimate_num_insns returns 0 for all internal function calls, so they implicitly count as is_simple_builtin (). Given we now have many GIMPLE-time IFNs this should probably be revisited with similar helpers we have for builtins or by making them also work for ifns via combined_fn handling. Richard. > Thanks, > Soumya > > > On 25 Aug 2026, at 1:20 PM, Soumya AR <[email protected]> wrote: > > > > Hi Jakub > > > > Thank you for the review! > > > > Sorry for the delay in getting back. I've addressed your points and added > > updated the patches with some more fixes. > > > > On on Tue, 21 Jul 2026 16:38:06 +0200, Jakub Jelinek <[email protected]> > > wrote: > >> On Wed, Jun 10, 2026 at 07:38:12PM +0530, [email protected] wrote: > >>> Builtins resolve in the frontend to a single internal function, > >>> > >>> IFN_ATOMIC_FETCH_MINMAX (ptr, value, memorder, is_min, zero_of_datatype) > >> > >> Why do you need zero_of_datatype, when you have the value argument? > >> Sure, I know pointer casts are useless, so you can't take the type from > >> ptr, but why can't you take the type from the value argument? > >> Make sure c-common.cc casts the value argument to TREE_TYPE (ptr), then > >> the type should be ok until lowering or expansion. > > > > Done. > > > > (The main motivation for doing this was that, during our initial FP add/sub > > work, we were using builtins and needed a constant to pass the type we cared > > about. We thought it would be better to keep this consistent across the > > IFNs. > > But since we are moving away from having builtins for FP, this is sort of > > moot.) > > > >> Also, I'd prefer memorder argument to come last, so > >> ptr, value, is_min, memorder. > >> > > > > Done. > > > >>> Signed-off-by: Soumya AR <[email protected]> > >> > >> This line belongs after ChangeLog entry, not before it. > >> > > > > Done. > > > >> I don't see documentation, the patch should contain gcc/doc/extend.texi > >> changes documenting the builtins, e.g. after __atomic_is_lock_free or so. > >> > > Added it at the end after __atomic_is_lock_free. Not sure if it should be > > included along with the rest of the __atomic_fetch_<op> builtins? > > > >>> --- a/gcc/Makefile.in > >>> +++ b/gcc/Makefile.in > >>> @@ -1421,6 +1421,7 @@ OBJS = \ > >>> alias.o \ > >>> alloc-pool.o \ > >>> asm-toplevel.o \ > >>> + atomic-ifn-lowering.o \ > >> > >> I'd prefer not to have a full new pass for this (even when it has a cfun-> > >> guard), but will defer to Richi about this. pass_gimple_isel is supposedly > >> too late, pass_optimize_widening_mul is -O1+ and non--Og only. > > > > Yeah that makes sense. I really don't know what the best approach is here. > > For now I have added the revised standalone pass as well. Will wait for > > Richi's > > opinion. > > > >> > >>> + /* Avoid lowering atomic IFN calls during the BB walk because lowering > >>> + can split basic blocks and corrupt the iterator. Collect the calls > >>> + and their matching descriptors first, then lower them one by one. > >>> */ > >>> + auto_vec<std::pair<gcall *, const atomic_op_lowering *> > to_lower; > >>> + FOR_EACH_BB_FN (bb, cfun) > >>> + for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); > >>> + gsi_next (&gsi)) > >>> + { > >>> + gcall *call = dyn_cast<gcall *> (gsi_stmt (gsi)); > >>> + if (!call) > >>> + continue; > >> > >> As you only handle internal calls, would be useful to continue > >> whenever !gimple_call_internal_p. > >> > > > > Done. > > > >>> + for (const atomic_op_lowering &op : atomic_op_table) > >>> + if (gimple_call_internal_p (call, op.ifn)) > >>> + { > >>> + to_lower.safe_push ({ call, &op }); > >>> + break; > >>> + } > >> > >> Why? Many passes manage this to handle it normally in the walk. > >> Pass &gsi to the lowering function and if changes are made, ensure to > >> set it to the stmt in the exit bb (if any). If needed, arrange for > >> gsi_next (&gsi) not to be done in that case. > >> > > > > Done. > > > >>> + /* sync_resolve_params expects a sized builtin decl; we only > >>> + register the generic form and lower via IFN, therefore, validate > >>> + params manually. */ > >>> + const char *err = NULL; > >>> + if (params->length () < 3) > >>> + err = "too few arguments to function %qE"; > >>> + else if (params->length () > 3) > >>> + err = "too many arguments to function %qE"; > >>> + else if (!INTEGRAL_TYPE_P (datatype)) > >>> + err = "argument 1 of %qE must point to an integer type"; > >>> + else if (!INTEGRAL_TYPE_P (TREE_TYPE ((*params)[1]))) > >>> + err = "argument 2 of %qE must be an integer type"; > >>> + else if (!INTEGRAL_TYPE_P (TREE_TYPE ((*params)[2]))) > >>> + err = "non-integer memory model argument 3 of %qE"; > >>> + if (err) > >>> + { > >>> + if (complain) > >>> + error_at (loc, err, function); > >>> + return error_mark_node; > >>> + } > >> > >> Don't do this, it is translation unfriendly. > >> Either mark them all with G_("..."); or better just use if (complain) > >> error_at; return error_mark_node; in each of those cases separately. > >> > > > > Added these separately. > > > >>> + /* Atomics can throw under -fnon-call-exceptions; mark the call > >>> + NOTHROW otherwise. */ > >>> + if (!flag_non_call_exceptions) > >>> + TREE_NOTHROW (ret) = 1; > >> > >> Do you handle -fnon-call-exceptions during lowering? How do you handle > >> it when expanded using hw insns? > >> > > > > My bad, this was not handled earlier. CAS lowering now attaches the new > > atomic > > load and CAS to the original IFN's LP. For hw insns, expand_gimple_stmt > > propagates the IFN's EH information to the trapping insns. > > > >>> +/* Expand atomic fetch minmax. */ > >>> + > >>> +static void > >>> +expand_ATOMIC_FETCH_MINMAX (internal_fn, gcall *) > >>> +{ > >>> + /* Implement this. */ > >> > >> 2 spaces rather than tab. > > > > Done. > > > >> > >>> +/* Each function must explicitly convert its argument to (int) (type of > >>> + global_val) before passing it to ATOMIC_FETCH_MINMAX. The four > >>> functions > >>> + each produce one such cast, so we expect exactly 4 in total. */ > >>> +/* { dg-final { scan-tree-dump-times "= \\(int\\) x" 4 "gimple" } } */ > >>> \ No newline at end of file > >> > >> Please avoid this. > >> > > > > My bad, fixed this across the tests now. > > > >>> @@ -328,7 +438,6 @@ test_or_fetch () > >>> abort (); > >>> } > >>> > >>> - > >>> /* Test the OP routines with a result which isn't used. Use both > >>> variations > >>> within each function. */ > >>> > >> > >> Why (several times)? > > > > Sorry, this was a leftover from removing the original op_fetch tests these > > patches > > had earlier. Fixed this. > > > > Thanks, > > Soumya > > > >> > >> Jakub > > > -- Richard Biener <[email protected]> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Jochen Jaser, Andrew McDonald, Abhinav Puri; (HRB 36809, AG Nuernberg)
