On Tue, Jun 02, 2026 at 04:49:49PM +0530, Avinash Jayakar wrote: Note, I'm not sure whether it is on your end or mine, but when I brought this reply into emacs, where it looked like a tab or something got turned into something else and my editor showed it as a glyph in green. I just deleted the sections where it showed up because it wasn't related to what I was replying to. It was mostly in quoting assembler text.
> > Hi Michael, > > Thanks for the review. I will update the patch and post a v4 with some > of the necessary changes you mentioned. However for some of points I > have some doubts/arguments below. Ok. I wasn't sure on some of them. > > You also need to add documentation to doc/extend.texi to describe > > these > > functions. > Noted, will update. > You are right, forcing it to register loads the addr into a separate > reg instead of using indexed form. Generated code for the above with O2 > is > > .LFB0: > .cfi_startproc > sldi 6,6,1 > add 8,3,6 > lhzx 10,4,6 > lhzx 7,5,6 > sync > lharx 9,3,6,1 > cmpw 0,9,10 > bne 0,.L2 > sthcx. 7,0,8 > .L2: > isync > mfcr 3,128 > rlwinm 3,3,3,1 > beq 0,.L3 > sthx 9,4,6 > > only for sthcx. it forced the addr into a register. I think for lhzx > and lharx, the rtl optimizations might have helped use the indexed form > of loads. It is a minor thing, but hey if it helps slightly it should be a win. > > > > I.e. the lwzx, lwarx, and stwcx. instructions use reg,reg addressing > > instead of 0,reg. I think using force_reg will tend to tell the > > optimizer to generate 0,reg address. > > > > The solution would be to switch to using copy_to_mode_reg instead of > > force_reg. I.e. > I tried using this, but it did not solve the issue mentioned above. > I looked at how generic builtin expands and they use expand_expr > instead of expand_normal. Replacing that seemed to solve the issue, > hope this change is OK. I haven't gone into that layer for awhile. > // For arg 0 (ptr to data) > rtx ptr = expand_expr (CALL_EXPR_ARG (exp, 0), NULL_RTX, Pmode, > EXPAND_SUM); > ptr = convert_memory_address (Pmode, ptr); > rtx mem = gen_rtx_MEM (mode, ptr); > > // For arg 1 (expected ptr) > rtx exp_ptr = expand_expr (CALL_EXPR_ARG (exp, 1), NULL_RTX, Pmode, > EXPAND_SUM); > exp_ptr = convert_memory_address (Pmode, exp_ptr); > rtx expected_val = gen_reg_rtx (mode); > emit_move_insn (expected_val, gen_rtx_MEM (mode, exp_ptr)); > > // For arg 2 (desired ptr) > rtx desired_ptr = expand_expr (CALL_EXPR_ARG (exp, 2), NULL_RTX, Pmode, > EXPAND_SUM); > desired_ptr = convert_memory_address (Pmode, desired_ptr); > rtx desired_val = gen_reg_rtx (mode); > emit_move_insn (desired_val, gen_rtx_MEM (mode, desired_ptr)); That is an alternative and it looks like it works. > Actually, it compiles fine without warning in all cases > signed/unsigned. We only look at the type size, and a fake builtin for > this is provided in the overload file, which takes void* as arguments > hence no parsing warnings. Only constraint on the types is Great. I just didn't want warnings about signed vs. unsigned types. > - 1st three argument type size must be same and known at compile time > (the types itself can be different) > > - They must be a complete pointer type. > > > Also, you will need to break the long lines. > > Sure will do, using '\' for line break should do right? I was facing > parsing errors building when I break them in new lines for the .def > files. Again, I haven't had been in that area of the code, but if '\' works that is fine. And if there is no way to break the line, we can probably allow the longer line. While I would prefer if we allowed longer lines, the coding standard does say to keep to the shorter line length. > > For both of the tests, could you add some tests where the EH field is > > 1 > > and not 0? And then check explicitly whether the expected 0 or 1 for > > EH is generated. > > Actually I'm a bit confused here. Are you referring to the 4th > argument? Actually that is for "bool weak" parameter same as > __atomic_compare_exchange. It has no influence on this particular > builtin as of now, and just added to keep the signatures of the new > builtin same as the generic __atomic_compare_exchange. Ah ok, I thought it was the EH field. Sorry about that. -- Michael Meissner, IBM PO Box 98, Ayer, Massachusetts, USA, 01432 email: [email protected]
