https://gcc.gnu.org/bugzilla/show_bug.cgi?id=127851
--- Comment #2 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
(In reply to H.J. Lu from comment #1)
> Created attachment 65820 [details]
> Disallow 2 volatile memory references.
> diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
> index 005c9ac7b20..27a40ec20c1 100644
> --- a/gcc/config/i386/i386.cc
> +++ b/gcc/config/i386/i386.cc
> @@ -17106,15 +17106,25 @@ ix86_match_ccmode (rtx insn, machine_mode req_mode)
> if (GET_CODE (set) == PARALLEL)
> set = XVECEXP (set, 0, 0);
> gcc_assert (GET_CODE (set) == SET);
> - gcc_assert (GET_CODE (SET_SRC (set)) == COMPARE);
> + rtx src = SET_SRC (set);
> + gcc_assert (GET_CODE (src) == COMPARE);
> +
> + rtx op0 = XEXP (src, 0);
> + rtx op1 = XEXP (src, 1);
> +
> + /* Disallow 2 volatile memory references. */
> + if (MEM_P (op0)
> + && MEM_VOLATILE_P (op0)
> + && MEM_P (op1)
> + && MEM_VOLATILE_P (op1))
> + return false;
>
> set_mode = GET_MODE (SET_DEST (set));
> switch (set_mode)
> {
> case E_CCNOmode:
> if (req_mode != CCNOmode
> - && (req_mode != CCmode
> - || XEXP (SET_SRC (set), 1) != const0_rtx))
> + && (req_mode != CCmode || op1 != const0_rtx))
> return false;
> break;
> case E_CCmode:
> @@ -17147,7 +17157,7 @@ ix86_match_ccmode (rtx insn, machine_mode req_mode)
> gcc_unreachable ();
> }
>
> - return GET_MODE (SET_SRC (set)) == set_mode;
> + return GET_MODE (src) == set_mode;
> }
Thanks. This fixes the cases in the PR, but (compare (mem) (mem)) isn't the
only way to get two volatile reads into one insn: *cmp<mode>_minus_1 takes
(compare (minus (mem) (mem)) (const_int 0)).
-O2 on x86_64:
```
extern volatile int x, y;
void g1 (void), g2 (void);
void sb (void)
{
int a = x;
int b = y;
if ((int) ((unsigned) a - (unsigned) b) < 0)
g1 ();
else
g2 ();
}
```
With patch above, combine still forms
8: flags:CCGOC=cmp([`x']-[`y'],0)
matched as *cmpsi_minus_1, giving
```
movl y(%rip), %eax
cmpl %eax, x(%rip)
js .L12
```
so y is read before x. Since -ffuse-ops-with-volatile-access is
target-independent, it would be better to plug this in combine itself.
Do you want me to post such a patch which I have tested locally ?
