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).
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
