On Fri, Oct 10, 2014 at 3:53 AM, Kyrill Tkachov <kyrylo.tkac...@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.tkac...@arm.com>
> Ramana Radhakrishnan<ramana.radhakrish...@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.
