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

[Andrew Pinski]

On Fri, Oct 10, 2014 at 3:53 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> 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:
This:
+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.

Thanks,
Andrew Pinski

>
> Thanks,
> Kyrill
>
> 2014-10-10  Kyrylo Tkachov<kyrylo.tkachov@arm.com>
>             Ramana Radhakrishnan<ramana.radhakrishnan@arm.com>
>
>      * 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.