On Wed, Nov 26, 2025 at 8:34 AM Jeff Law <[email protected]> wrote:
>
>
>
> On 11/20/25 4:35 PM, H.J. Lu wrote:
>
> >>
> >> 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.
> But that really should be a property of the target's patterns, operand
> predicates and such. I still don't think that's a good reason at all to
> have a target hook. Either we preserve semantics or we do not. If we
> preserve semantics, then this should work everyone (sans any target bugs
> which would be just that, bugs).
I will remove the target hook.
>
> >
> > 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.
> But if we make stores work sensibly, then we no longer need the
> instruction, right? It's the mere need to track the instruction that
> seems rather hokey/hackish to me right now.
>
> jeff
For
volatile unsigned char u8;
void test (void)
{
u8 = u8 + u8;
u8 = u8 - u8;
}
When volatile store is allowed, we generate
(insn 8 7 9 2 (parallel [
(set (mem/v/c:QI (symbol_ref:DI ("u8") [flags 0x2]
<var_decl 0x7fe9719d6e40 u8>) [0 u8+0 S1 A8])
(ashift:QI (mem/v/c:QI (symbol_ref:DI ("u8") [flags
0x2] <var_decl 0x7fe9719d6e40 u8>) [0 u8+0 S1 A8])
(const_int 1 [0x1])))
(clobber (reg:CC 17 flags))
])
"/export/gnu/import/git/gitlab/x86-gcc-test/gcc/testsuite/gcc.dg/pr86617.c":7:6
1139 {*ashlqi3_1}
(expr_list:REG_UNUSED (reg:CC 17 flags)
(nil)))
on x86 which leads to
salb u8(%rip)
instead of 2 loads. Without the instruction, we don't know if a
memory reference
should be allowed for stores.
--
H.J.
