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
