This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][AArch64] Implement workaround for ARM Cortex-A53 erratum 835769

On Fri, Oct 10, 2014 at 3:53 AM, Kyrill Tkachov <> wrote:
> Hi all,
> Some early revisions of the Cortex-A53 have an erratum (835769) whereby
> it is possible for a 64-bit multiply-accumulate instruction in
> AArch64 state to generate an incorrect result.  The details are quite
> complex and hard to determine statically, since branches in the code
> may exist in some circumstances, but all cases end with a memory
> (load, store, or prefetch) instruction followed immediately by the
> multiply-accumulate operation.
> The safest work-around for this issue is to make the compiler avoid
> emitting multiply-accumulate instructions immediately after memory
> instructions and the simplest way to do this is to insert a NOP. A
> linker patching technique can also be used, by moving potentially
> affected multiply-accumulate instruction into a patch region and
> replacing the original instruction with a branch to the patch.
> This patch achieves the compiler part by using the final prescan pass.
> The insn length attribute is updated accordingly using ADJUST_INSN_LENGTH
> so that conditional branches work properly.
> The fix is disabled by default and can be turned on by the
> -mfix-cortex-a53-835769 compiler option.
> I'm attaching a trunk and a 4.9 version of the patch.
> The 4.9 version is different only in that rtx_insn* is replaced by rtx.
> Tested on aarch64-none-linux-gnu (and bootstrap with the option) and
> built various large benchmarks with it.
> Ok?

A few comments:
+static int
+is_mem_p (rtx *x, void *data ATTRIBUTE_UNUSED)
+  return MEM_P (*x);
+static bool
+is_memory_op (rtx_insn *mem_insn)
+   rtx pattern = PATTERN (mem_insn);
+   return for_each_rtx (&pattern, is_mem_p, NULL);

Should be using the new FOR_EACH_SUBRTX instead.  This should simplify
the code something like:

static bool
is_memory_op (rtx_insn *mem_insn)
  subrtx_iterator::array_type array;
  FOR_EACH_SUBRTX (iter, array, PATTERN (mem_insn), ALL)
    if (MEM_P (*iter))
      return true;
  return false;

Also this should be a has_ function rather than a is_ function.

I think you should have ADJUST_INSN_LENGTH as a function call rather
than inline it there or at least wrap it with do { } while (0);  which
I think is documented part of the coding style.

Also you added no comment before each function.  The coding style says
each function needs a comment describing what the function does.

Andrew Pinski

> Thanks,
> Kyrill
> 2014-10-10  Kyrylo Tkachov<>
>             Ramana Radhakrishnan<>
>      * config/aarch64/aarch64.h (FINAL_PRESCAN_INSN): Define.
>      (ADJUST_INSN_LENGTH): Define.
>      * config/aarch64/aarch64.opt (mfix-cortex-a53-835769): New option.
>      * config/aarch64/aarch64.c (is_mem_p): New function.
>      (is_memory_op): Likewise.
>      (aarch64_prev_real_insn): Likewise.
>      (is_madd_op): Likewise.
>      (dep_between_memop_and_next): Likewise.
>      (aarch64_madd_needs_nop): Likewise.
>      (aarch64_final_prescan_insn): Likewise.
>      * doc/invoke.texi (AArch64 Options): Document -mfix-cortex-a53-835769
>      and -mno-fix-cortex-a53-835769 options.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]