On 10/22/26 5:23 PM, Vineet Gupta wrote:
I don't want to jump the gun on the due review process but seems like
there is some feedback from gcc sashiko which Uroz shared with me [1]
and the issue it found seems legit.
Obviously being an LLM, you have to look at what it says with a
skeptical eye and being clean with GCC Sashiko isn't in and of itself a
good indicator that a patch is ready. But with those caveats out of the
way, absolutely happy to see you're proactively looking at its output
and taking steps to determine if what it's saying is a real issue or not.
[1]
https://gcc.sashiko.dev/#/patchset/20261021211758.3144964-1-vineet.gupta%40linux.dev
Essentially the "succ == 0" check above implies this only happens for
I2 -> I3 combine, but leaves the 3 (or 4) combine cases broken. e.g.
Test below on x86, z is incorrectly read before y (with my v4 and
despite Uroz's i386.md fix)
extern volatile int x, y, z;
int noarg (void) { int t1 = x; int t2 = y; int t3 = z; return t1 +
t2 + t3; }
Dropping the check fixes this test above but regresses other valid
cases: the test from original introduction of
-ffuse-ops-with-volatile-access.
The issue is can_combine_p only able to see the inputs while ensuing
failing to recog and split could re-introduce the reordering. So the
fix is to check later, after any splits (with a pre-scan recording the
volatiles).
Is this something that seems like a right approach for a v5 ?
It does. Certainly fixing for 2->1 is an improvement, but 3->1, 4->1 as
well as 3->2 and 4->2 splits can certainly occur and one could even
speculate that they're more likely than 2->1 to result in multiple
volatile accesses and trigger the problems we're trying to avoid.
Good to see that it gave you an attempt at a testcase too. I need to
add that to machine description review procedures so that the LLM
automatically tries to generate a suitable testcase for issues it
finds. That checker did find a code gen issue in bitmanip.md today and
even spit out a nice little testcase when asked.
jeff
