[AArch64] Use Feature for A53 Erratum 835769 Fix

[lenary]

Summary

When this pass was originally implemented, the fix pass was enabled
using a llvm command-line flag. This works fine, except in the case of
LTO, where the flag is not passed into the linker plugin in order to
enable the function pass in the LTO backend.

Now LTO exists, the expectation now is to use target features rather
than command-line arguments to control code generation, as this ensures
that different command-line arguments in different files are correctly
represented, and target-features always get to the LTO plugin as they
are encoded into LLVM IR.

The fall-out of this change is that the fix pass has to always be added
to the backend pass pipeline, so now it makes no changes if the function
does not have the right target feature to enable it. This should make a
minimal difference to compile time.

One advantage is it's now much easier to enable when compiling for a
Cortex-A53, as CPUs imply their own individual sets of target-features,
in a more fine-grained way. I haven't done this yet, but it is an
option, if the fix should be enabled in more places.

Existing tests of the user interface are unaffected, the changes are to
reflect that the argument is now turned into a target feature.

[tmatheson]

Makes sense to me

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

fp16_fml_fallthrough:
   if (Args.hasArg(options::OPT_fcall_saved_x15))
     Features.push_back("+call-saved-x15");
 
   if (Args.hasArg(options::OPT_fcall_saved_x18))
     Features.push_back("+call-saved-x18");
 
   if (Args.hasArg(options::OPT_mno_neg_immediates))
     Features.push_back("+no-neg-immediates");
+
+  if (Arg *A = Args.getLastArg(options::OPT_mfix_cortex_a53_835769,
+                               options::OPT_mno_fix_cortex_a53_835769)) {
+    if (A->getOption().matches(options::OPT_mfix_cortex_a53_835769))
+      Features.push_back("+fix-cortex-a53-835769");
+    else
+      Features.push_back("-fix-cortex-a53-835769");
+  } else if (Triple.isAndroid()) {
+    // Enabled A53 errata (835769) workaround by default on android
+    Features.push_back("+fix-cortex-a53-835769");
+  }
 }

clang/lib/Driver/ToolChains/Clang.cpp

if (!Args.hasFlag(options::OPT_mred_zone, options::OPT_mno_red_zone, true) ||
     CmdArgs.push_back("-disable-red-zone");
 
   if (!Args.hasFlag(options::OPT_mimplicit_float,
                     options::OPT_mno_implicit_float, true))
     CmdArgs.push_back("-no-implicit-float");
 
   RenderAArch64ABI(Triple, Args, CmdArgs);
 
-  if (Arg *A = Args.getLastArg(options::OPT_mfix_cortex_a53_835769,
-                               options::OPT_mno_fix_cortex_a53_835769)) {
-    CmdArgs.push_back("-mllvm");
-    if (A->getOption().matches(options::OPT_mfix_cortex_a53_835769))
-      CmdArgs.push_back("-aarch64-fix-cortex-a53-835769=1");
-    else
-      CmdArgs.push_back("-aarch64-fix-cortex-a53-835769=0");
-  } else if (Triple.isAndroid()) {
-    // Enabled A53 errata (835769) workaround by default on android
-    CmdArgs.push_back("-mllvm");
-    CmdArgs.push_back("-aarch64-fix-cortex-a53-835769=1");
-  }
-
   // Forward the -mglobal-merge option for explicit control over the pass.
   if (Arg *A = Args.getLastArg(options::OPT_mglobal_merge,
                                options::OPT_mno_global_merge)) {
     CmdArgs.push_back("-mllvm");
     if (A->getOption().matches(options::OPT_mno_global_merge))
       CmdArgs.push_back("-aarch64-enable-global-merge=false");
     else
       CmdArgs.push_back("-aarch64-enable-global-merge=true");

clang/test/Driver/aarch64-fix-cortex-a53-835769.c

 // RUN: %clang -target aarch64-linux-eabi %s -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-DEF %s
 // RUN: %clang -target aarch64-linux-eabi -mfix-cortex-a53-835769 %s -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-YES %s
 // RUN: %clang -target aarch64-linux-eabi -mno-fix-cortex-a53-835769 %s -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO %s
 
 // RUN: %clang -target aarch64-android-eabi %s -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-YES %s
 
-// CHECK-DEF-NOT: "-mllvm" "-aarch64-fix-cortex-a53-835769"
-// CHECK-YES: "-mllvm" "-aarch64-fix-cortex-a53-835769=1"
-// CHECK-NO: "-mllvm" "-aarch64-fix-cortex-a53-835769=0"
+// CHECK-DEF-NOT: "{[+-]}fix-cortex-a53-835769"
+// CHECK-YES: "+fix-cortex-a53-835769"
+// CHECK-NO: "-fix-cortex-a53-835769"

llvm/lib/Target/AArch64/AArch64.td

def FeatureAppleA7SysReg  : SubtargetFeature<"apple-a7-sysreg", "HasAppleA7SysReg", "true",
   "Apple A7 (the CPU formerly known as Cyclone)">;
 
 def FeatureEL2VMSA : SubtargetFeature<"el2vmsa", "HasEL2VMSA", "true",
   "Enable Exception Level 2 Virtual Memory System Architecture">;
 
 def FeatureEL3 : SubtargetFeature<"el3", "HasEL3", "true",
   "Enable Exception Level 3">;
 
+def FeatureFixCortexA53_835769 : SubtargetFeature<"fix-cortex-a53-835769",
+  "FixCortexA53_835769", "true", "Mitigate Cortex-A53 Erratum 835769">;
+
 //===----------------------------------------------------------------------===//
 // Architectures.
 //
 def HasV8_0aOps : SubtargetFeature<"v8a", "HasV8_0aOps", "true",
   "Support ARM v8.0a instructions", [FeatureEL2VMSA, FeatureEL3]>;
 
 def HasV8_1aOps : SubtargetFeature<"v8.1a", "HasV8_1aOps", "true",
   "Support ARM v8.1a instructions", [HasV8_0aOps, FeatureCRC, FeatureLSE,

llvm/lib/Target/AArch64/AArch64A53Fix835769.cpp

 // in some circumstances may trigger the erratum.
 // It inserts a nop instruction between a sequence of the following 2 classes
 // of instructions:
 // instr 1: mem-instr (including loads, stores and prefetches).
 // instr 2: non-SIMD integer multiply-accumulate writing 64-bit X registers.
 //===----------------------------------------------------------------------===//
 
 #include "AArch64.h"
+#include "AArch64Subtarget.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/Support/Debug.h"

 INITIALIZE_PASS(AArch64A53Fix835769, "aarch64-fix-cortex-a53-835769-pass",
                 "AArch64 fix for A53 erratum 835769", false, false)
 
 //===----------------------------------------------------------------------===//
 
 bool
 AArch64A53Fix835769::runOnMachineFunction(MachineFunction &F) {
   LLVM_DEBUG(dbgs() << "***** AArch64A53Fix835769 *****\n");
+  auto &STI = F.getSubtarget<AArch64Subtarget>();
+  // Fix not requested, skip pass.
+  if (!STI.fixCortexA53_835769())
+    return false;
+
   bool Changed = false;
-  TII = F.getSubtarget().getInstrInfo();
+  TII = STI.getInstrInfo();
 
   for (auto &MBB : F) {
     Changed |= runOnBasicBlock(MBB);
   }
   return Changed;
 }
 
 // Return the block that was fallen through to get to MBB, if any,

llvm/lib/Target/AArch64/AArch64Subtarget.h

protected:
   bool HasLSE2 = false;
   bool HasRAS = false;
   bool HasRDM = false;
   bool HasPerfMon = false;
   bool HasFullFP16 = false;
   bool HasFP16FML = false;
   bool HasSPE = false;
 
+  bool FixCortexA53_835769 = false;
+
   // ARMv8.1 extensions
   bool HasVH = false;
   bool HasPAN = false;
   bool HasLOR = false;
 
   // ARMv8.2 extensions
   bool HasPsUAO = false;
   bool HasPAN_RWV = false;
public:
   bool hasLS64() const { return HasLS64; }
   bool hasSEL2() const { return HasSEL2; }
   bool hasTLB_RMI() const { return HasTLB_RMI; }
   bool hasFlagM() const { return HasFlagM; }
   bool hasRCPC_IMMO() const { return HasRCPC_IMMO; }
   bool hasEL2VMSA() const { return HasEL2VMSA; }
   bool hasEL3() const { return HasEL3; }
 
+  bool fixCortexA53_835769() const { return FixCortexA53_835769; }
+
   bool addrSinkUsingGEPs() const override {
     // Keeping GEPs inbounds is important for exploiting AArch64
     // addressing-modes in ILP32 mode.
     return useAA() || isTargetILP32();
   }
 
   bool useSmallAddressing() const {
     switch (TLInfo.getTargetMachine().getCodeModel()) {

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp

EnableEarlyIfConversion("aarch64-enable-early-ifcvt", cl::Hidden,
                         cl::init(true));
 
 static cl::opt<bool>
     EnableCondOpt("aarch64-enable-condopt",
                   cl::desc("Enable the condition optimizer pass"),
                   cl::init(true), cl::Hidden);
 
 static cl::opt<bool>
-EnableA53Fix835769("aarch64-fix-cortex-a53-835769", cl::Hidden,
-                cl::desc("Work around Cortex-A53 erratum 835769"),
-                cl::init(false));
-
-static cl::opt<bool>
     EnableGEPOpt("aarch64-enable-gep-opt", cl::Hidden,
                  cl::desc("Enable optimizations on complex GEPs"),
                  cl::init(false));
 
 static cl::opt<bool>
     BranchRelaxation("aarch64-enable-branch-relax", cl::Hidden, cl::init(true),
                      cl::desc("Relax out of range conditional branches"));
 

 
 void AArch64PassConfig::addPreEmitPass() {
   // Machine Block Placement might have created new opportunities when run
   // at O3, where the Tail Duplication Threshold is set to 4 instructions.
   // Run the load/store optimizer once more.
   if (TM->getOptLevel() >= CodeGenOpt::Aggressive && EnableLoadStoreOpt)
     addPass(createAArch64LoadStoreOptimizationPass());
 
-  if (EnableA53Fix835769)
-    addPass(createAArch64A53Fix835769());
 
   if (EnableBranchTargets)
     addPass(createAArch64BranchTargetsPass());
 
   // Relax conditional branch instructions if they're otherwise out of
   // range of their destination.
   if (BranchRelaxation)
     addPass(&BranchRelaxationPassID);

llvm/test/CodeGen/AArch64/O0-pipeline.ll

 ; CHECK-NEXT:       AArch64 pseudo instruction expansion pass
 ; CHECK-NEXT:       AArch64 speculation hardening pass
 ; CHECK-NEXT:       AArch64 Indirect Thunks
 ; CHECK-NEXT:       AArch64 sls hardening pass
 ; CHECK-NEXT:       Analyze Machine Code For Garbage Collection
 ; CHECK-NEXT:       Insert fentry calls
 ; CHECK-NEXT:       Insert XRay ops
 ; CHECK-NEXT:       Implement the 'patchable-function' attribute
+; CHECK-NEXT:       Workaround A53 erratum 835769 pass
 ; CHECK-NEXT:       AArch64 Branch Targets
 ; CHECK-NEXT:       Branch relaxation pass
 ; CHECK-NEXT:       Contiguously Lay Out Funclets
 ; CHECK-NEXT:       StackMap Liveness Analysis
 ; CHECK-NEXT:       Live DEBUG_VALUE analysis
 ; CHECK-NEXT:       Unpack machine instruction bundles
 ; CHECK-NEXT:       Lazy Machine Block Frequency Analysis
 ; CHECK-NEXT:       Machine Optimization Remark Emitter
 ; CHECK-NEXT:       AArch64 Assembly Printer
 ; CHECK-NEXT:       Free MachineFunction
 
 define void @f() {
   ret void
 }

llvm/test/CodeGen/AArch64/O3-pipeline.ll

 ; CHECK-NEXT:       Analyze Machine Code For Garbage Collection
 ; CHECK-NEXT:       Machine Block Frequency Analysis
 ; CHECK-NEXT:       MachinePostDominator Tree Construction
 ; CHECK-NEXT:       Branch Probability Basic Block Placement
 ; CHECK-NEXT:       Insert fentry calls
 ; CHECK-NEXT:       Insert XRay ops
 ; CHECK-NEXT:       Implement the 'patchable-function' attribute
 ; CHECK-NEXT:       AArch64 load / store optimization pass
+; CHECK-NEXT:       Workaround A53 erratum 835769 pass
 ; CHECK-NEXT:       AArch64 Branch Targets
 ; CHECK-NEXT:       Branch relaxation pass
 ; CHECK-NEXT:       AArch64 Compress Jump Tables
 ; CHECK-NEXT:       Contiguously Lay Out Funclets
 ; CHECK-NEXT:       StackMap Liveness Analysis
 ; CHECK-NEXT:       Live DEBUG_VALUE analysis
 ; CHECK-NEXT:     Machine Outliner
 ; CHECK-NEXT:     FunctionPass Manager

llvm/test/CodeGen/AArch64/aarch64-fix-cortex-a53-835769.ll

 ; REQUIRES: asserts
 ; The regression tests need to test for order of emitted instructions, and
 ; therefore, the tests are a bit fragile/reliant on instruction scheduling. The
 ; test cases have been minimized as much as possible, but still most of the test
 ; cases could break if instruction scheduling heuristics for cortex-a53 change
-; RUN: llc < %s -mcpu=cortex-a53 -aarch64-fix-cortex-a53-835769=1 -frame-pointer=non-leaf -stats 2>&1 \
+; RUN: llc < %s -mcpu=cortex-a53 -mattr=+fix-cortex-a53-835769 -frame-pointer=non-leaf -stats 2>&1 \
 ; RUN:  | FileCheck %s
-; RUN: llc < %s -mcpu=cortex-a53 -aarch64-fix-cortex-a53-835769=0 -frame-pointer=non-leaf -stats 2>&1 \
+; RUN: llc < %s -mcpu=cortex-a53 -mattr=-fix-cortex-a53-835769 -frame-pointer=non-leaf -stats 2>&1 \
 ; RUN:  | FileCheck %s --check-prefix CHECK-NOWORKAROUND
 ; The following run lines are just to verify whether or not this pass runs by
 ; default for given CPUs. Given the fragility of the tests, this is only run on
 ; a test case where the scheduler has not freedom at all to reschedule the
 ; instructions, so the potentially massively different scheduling heuristics
 ; will not break the test case.
 ; RUN: llc < %s -mcpu=generic    -frame-pointer=non-leaf | FileCheck %s --check-prefix CHECK-BASIC-PASS-DISABLED
 ; RUN: llc < %s -mcpu=cortex-a53 -frame-pointer=non-leaf | FileCheck %s --check-prefix CHECK-BASIC-PASS-DISABLED