On Thu, Nov 20, 2025 at 8:08 AM Jeff Law <[email protected]> wrote: > > > > On 11/16/25 4:36 PM, H.J. Lu wrote: > > On Mon, Nov 17, 2025 at 12:38 AM Jeff Law <[email protected]> wrote: > >> > >> > >> > >> On 10/27/25 3:53 PM, H.J. Lu wrote: > >> > >>> > >>>> As HJ indicated, I wouldn't expect significant fallout on load/store > >>>> architectures. So the scope of fallout isn't likely to be too terrible. > >>>> > >>>> I was going to throw it in my tester, but it doesn't apply cleanly on > >>>> the trunk. > >>>> > >>>>> patching file gcc/recog.cc > >>>>> Hunk #1 FAILED at 116. > >>>>> 1 out of 2 hunks FAILED -- saving rejects to file gcc/recog.cc.rej > >>>> > >>> > >>> I put my branch at > >>> > >>> https://gitlab.com/x86-gcc/gcc/-/commits/users/hjl/pr122343/master > >>> > >>> on top of > >>> > >>> commit 76943639ddd861dce3886d1def2a353ccfcdd585 > >>> Author: Eric Botcazou <[email protected]> > >>> Date: Mon Oct 27 19:51:11 2025 +0100 > >>> > >>> Ada: Fix assertion failure on child generic package > >>> > >>> Does it work for you? > >> So I generated a diff from that and put it into my tester with no > >> fallout. But that's because it's disabled unless you have a suitable > >> hook defined for the target. > >> > >> It would be helpful (and on the right track for integration) if the > >> branch just enabled it by default. > >> > > > > Here is the branch: > > > > https://gitlab.com/x86-gcc/gcc/-/commits/users/hjl/pr122343/v3 > > > > to enable it by default. > So that's been in my tester for a few days with zero fallout. And > AFAICT it is on by default (verified on the H8 which can do memory > operations like addition/subtraction). > > I don't have a high degree of confidence in coverage of volatile > accesses in the testsuite. But I also don't have any concrete evidence > of where this patch is likely to cause problems. > > The latest version needs to be posted for review, but before you do > that, I few comments on the version I tested from your branch. > > I don't see any good reason to have a target hook to disable this > behavior since we have a flag to control. If the patch truly preserves > volatile semantics, then it should work everywhere and target > disablement seems short-sighted. > > Is it really necessary to keep that "try_combine_insn" object? It seems > like you only use that to verify that the insn being recognized is a > load, right? So if you supported stores you wouldn't really needed it, > right?
The current patch only allows volatile memory load ops. We may allow write-after-read for volatile memory store ops. A target hook is handy if only some targets support them or a subset of store ops. The reasons for try_combine_insn are that 1. When general_operand is called, instruction info isn't available. 2. Only recog_for_combine_1 calls init_recog_no_volatile and tries memory operands. -- H.J.
