On on Fri, 28 Aug 2026 13:18:08 +0200 (CEST), Richard Biener <[email protected]> wrote: > 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. >
Hi, Thank you for the quick reply! Yes, I did not see any optimizations run on the CAS loop itself. The decision to use GIMPLE rather than RTL expansion was largely based on the potential optimizations on the CAS loop. (https://gcc.gnu.org/pipermail/gcc-patches/2025-November/699611.html) Even if we're not observing those in practice, there is the added benefit of being able to lower to a CAS loop in GIMPLE with TSan instead of saying “Sorry, unsupported.” Is it OK to go ahead with isel then? >> --- >> >> 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. Ah, this makes sense now. I experimented with adding costing for IFN_ATOMIC_FETCH_MINMAX, which fixes this. Can that be a separate fix as part of this series, or should we generalize the costing for all GIMPLE-time IFNs first? Thanks, Soumya > > 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)
