On Tue, Oct 28, 2025 at 12:04 AM Jeff Law <[email protected]> wrote:
>
>
>
> On 10/27/25 2:37 AM, Richard Biener wrote:
> ?
> >>
> >> There are 2 reasons:
> >>
> >> 1. RISC processors only allow moves with memory. Since this optimization
> >> only applies to other load operations, it is nop for most RISC processors.
> >> 2. Some GCC tests scan compiler assembly outputs which don't expect other
> >> load operations with volatile memory. I have to disable this optimization
> >> in 100+ x86 tests in my patch. If this is enabled for other targets, it
> >> may
> >> cause many GCC tests failures.
> >>
> >> If your target supports this, you can give it a try.
> >>
> >>> Can it break volatile correctness or so?
> >>
> >> No. My patch has some tests to verify the volatile correctness.
> >
> > IMO the -fcombine-op-with-volatile-memory-load is quite a bad name.
> > Options should have names that mean something to users. As this seems to
> > supposed to be a target opt-in I'd suggest to not expose this to users
> > (or if targets are concerned, via some -m...) but instead make this a
> > target hook/macro. Of course I think this should be enabled by default.
>
> If we're going to go with this patch or a variant thereof; I'd go a step
> further and not bother with a target hook. If the semantics are wrong
> for some case, we're more likely to find it the more targets use the
> option. And it sounds like the main purpose behind the hook is to avoid
> working through any testsuite issues on those other targets, which isn't
> a great reason to introduce a new target hook.
One advantage of a target hook is that if a target doesn't opt in, there
no need to do additional check:
diff --git a/gcc/recog.cc b/gcc/recog.cc
index 67d7fa63069..fc411842859 100644
--- a/gcc/recog.cc
+++ b/gcc/recog.cc
@@ -1596,7 +1596,8 @@ general_operand (rtx op, machine_mode mode)
{
rtx y = XEXP (op, 0);
- if (! volatile_ok && MEM_VOLATILE_P (op))
+ if (!targetm.volatile_mem_ok_in_insn (op,
+ recog_data.try_combine_insn))
return false;
/* Use the mem's mode, since it will be reloaded thus. LRA can
diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc
index 1873d572ba3..ece1f496571 100644
--- a/gcc/targhooks.cc
+++ b/gcc/targhooks.cc
@@ -1839,6 +1839,14 @@ default_new_address_profitable_p (rtx memref
ATTRIBUTE_UNUSED,
return true;
}
+/* The default implementation of TARGET_VOLATILE_MEM_OK_IN_INSN. */
+
+bool
+default_volatile_mem_ok_in_insn (rtx memref, rtx_insn *)
+{
+ return volatile_ok || !MEM_VOLATILE_P (memref);
+}
+
> 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?
Thanks.
--
H.J.
