[AArch64] Refactor the scheduling predicates (1/3) (NFC)

[evandro]

Summary

Refactor the scheduling predicates based on MCInstPredicate. In this case, AArch64InstrInfo::isScaledAddr().

[andreadb]

Hi Evandro.

Thanks for the patch. However, it is quite big to review.

Can you split it into multiple patches?

I think you can start by just rewiting method isScaledAddr.
It would be easier for me (and other reviewers) to see the differences in the auto-generated .inc file.

About isScaledAddr:
You can use a MCOpcodeSwitchStatement and rewrite that definition to something like this:

// Common variants to various AArch64 processor models.

let FunctionMapper = "AArch64_AM::getMemExtendType" in
def CheckExtendType : CheckImmOperand_s<3, "AArch64_AM::UXTX">;

let FunctionMapper = "AArch64_AM::getMemDoShift" in
def CheckFoldedShift : CheckImmOperandSimple<3>;

// ScaledIdxPred is true if a WriteLDIdx operand will be
// scaled. Subtargets can use this to dynamically select resources and
// latency for WriteLDIdx and ReadAdrBase.
//
// Method 'isScaledAddr' Returns true if this is load/store scales or extends
// its register offset. This refers to scaling a dynamic index as opposed to
// scaled immediates.  MI should be a memory op that allows scaled addressing.
def ScaledIdxPred : TIIPredicate<"isScaledAddr",
  MCOpcodeSwitchStatement<[
    MCOpcodeSwitchCase<[
        LDRBBroW, LDRBroW, LDRDroW,
        LDRHHroW, LDRHroW, LDRQroW,
        LDRSBWroW, LDRSBXroW, LDRSHWroW, LDRSHXroW,
        LDRSWroW, LDRSroW, LDRWroW, LDRXroW,

        STRBBroW, STRBroW, STRDroW,
        STRHHroW, STRHroW, STRQroW,
        STRSroW, STRWroW, STRXroW,

        LDRBBroX, LDRBroX, LDRDroX,
        LDRHHroX, LDRHroX, LDRQroX,
        LDRSBWroX, LDRSBXroX, LDRSHWroX, LDRSHXroX,
        LDRSWroX, LDRSroX, LDRWroX, LDRXroX,

        STRBBroX, STRBroX, STRDroX,
        STRHHroX, STRHroX, STRQroX,
        STRSroX, STRWroX, STRXroX
      ],
      MCReturnStatement<CheckAny<[
        CheckNot<CheckExtendType>,
        CheckFoldedShift ]>>
    >], MCReturnStatement<FalsePred>
  >
>;

The resulting code expanded by tablegen should be more readable (opcodes appear as cases of a switch-stmt).

I sugges that you to split this NFC patch into multiple patches.
To start, you can send a patch that rewrites isScaledAddr only, and then verify that the auto-generated definition in the aarch64 gen-instrinfo .inc file is still semantically correct.
You should be able to add llvm-mca tests to verify that writes which require isScaledAddr to resolve their scheduling class are now correctly analyzed by llvm-mca.

-Andrea

[evandro]

Awesome! Please, stand by.

[evandro]

Question: the code beginning at MCOpcodeSwitchStatement above cannot be used as a regular MCSchedPredicate too. If so, how can I avoid writing the same statement twice, since this condition is used both in AArch64InstrInfo.cpp and in AArch64Sched*.td?

[andreadb]

In D54777#1305675, @evandro wrote:

Question: the code beginning at MCOpcodeSwitchStatement above cannot be used as a regular MCSchedPredicate too. If so, how can I avoid writing the same statement twice, since this condition is used both in AArch64InstrInfo.cpp and in AArch64Sched*.td?

You can use a TIIPredicate definition to model an MCSchedPredicate.
The MCOpcodeSwitchStatement used by the ScaledIdxPred definition from my previous comment is effectively defining the body of method "isScaledAddr".
ScaledIdxPred can be used in MCSchedPredicate definitions.

Example. In AArch64SchedExynosM1 you could have:

-def M1ReadAdrBase : SchedReadVariant<[SchedVar<ScaledIdxPred, [ReadDefault]>,
-                                      SchedVar<NoSchedPred,   [ReadDefault]>]>;
+def M1ReadAdrBase : SchedReadVariant<[
+    SchedVar<MCSchedPredicate<ScaledIdxPred>, [ReadDefault]>,
+    SchedVar<NoSchedPred,                     [ReadDefault]>
+]>;

A TIIPredicate used in a MCSchedPredicate definition is always expanded into a TII method call.

That being said, I am not sure if we are talking about the same thing...

[evandro]

Break up the original patch in 3 installments, one for each existing predicate: AArch64InstrInfo::isScaledAddr(), AArch64InstrInfo::hasShiftedReg(), AArch64InstrInfo::hasExtendedReg().

[andreadb]

llvm/lib/Target/AArch64/AArch64SchedPredicates.td

62–63

If IsLoadRegOffsetPred and IsStoreRegOffsetPred are only used by the definition of ScaledIdxBody, then you should simply use a MCOpcodeSwitchStatement.

I had a look at your three NFC patches; this is the only place where you use IsLoadRegOffsetPred and IsStoreRegOffsetPred.

I know that in one of your previous comments you mentioned a prolem with MCOpcodeSwitchStatement. I still don't understand what the problem is. When I tried to rewrite your patch using MCOpcodeSwitchStatement everything worked fine for me...
I wonder if the issue is caused by the way you defined ScaledIdxPred. That definition should be changed to reference ScaledIdxFn directly (See my comment below).

66–68

ScaledIdxPred should be defined as MCSchedPredicate<ScaleIdxFn>.
Otherwise, you would end up expanding the entire body of 'isScaledAddr` inline; it won't be expanded into a TII call to isScaledAddr().

So, the definitions should be:

def ScaledIdxFn   : TIIPredicate<"isScaledAddr",
                                 MCReturnStatement<ScaledIdxBody>>;
def ScaledIdxPred : MCSchedPredicate<ScaledIdxFn>;

As I mentioned in one of my comments from yesterday, a TIIPredicate is-a MCInstPredicate.
When used in the defintion of a MCSchedPredicate, it is expanded by the PredicateExpander (see llvm/utils/Tablegen/PredicateExpander.{h,cpp}) into a TII call.

I wonder if this is the main reason why you think that using a MCOpcodeSwitchStatement in the definition of ScaledIdxBody is problematic ...

[evandro]

llvm/lib/Target/AArch64/AArch64SchedPredicates.td

62–63

Yes, I intend to reuse both IsLoadRegOffsetPred and IsStoreRegOffsetPred in a future patch. This is the reason; nothing wrong with using a more elegant switch statement in the emitted code.

66–68

Neat!

[andreadb]

llvm/lib/Target/AArch64/AArch64SchedPredicates.td

62–63

I think that you can use a !listconcat operator to concatenate the opcodes from those two predicates.

Something like this (I didn't verify that it works though...):

def ScaledIdxFn : TIIPredicate<"isScaledAddr",
  MCOpcodeSwitchStatement<[
    MCOpcodeSwitchCase<
      !listconcat(IsLoadRegOffsetPred.ValidOpcodes, IsStoreRegOffsetPred.ValidOpcodes),
      MCReturnStatement<CheckAny<[
        CheckNot<CheckExtendType>,
        CheckFoldedShift ]>>
    >], MCReturnStatement<FalsePred>
  >
>;

I think it should work. It would let you use the switch statement.

[andreadb]

llvm/lib/Target/AArch64/AArch64SchedPredicates.td

62–63

By the way. If !listconcat works for you (and you are happy with it), then you can use it to improve the other two NFC patches too.

[evandro]

llvm/lib/Target/AArch64/AArch64SchedPredicates.td

62–63

It seems to work. Will do.

[andreadb]

LGTM. Thanks!

llvm/lib/Target/AArch64/AArch64.td

 include "AArch64CallingConvention.td"
 
 //===----------------------------------------------------------------------===//
 // Instruction Descriptions
 //===----------------------------------------------------------------------===//
 
 include "AArch64Schedule.td"
 include "AArch64InstrInfo.td"
+include "AArch64SchedPredicates.td"
 
 def AArch64InstrInfo : InstrInfo;
 
 //===----------------------------------------------------------------------===//
 // Named operands for MRS/MSR/TLBI/...
 //===----------------------------------------------------------------------===//
 
 include "AArch64SystemOperands.td"

llvm/lib/Target/AArch64/AArch64InstrInfo.h

public:
   static bool isGPRZero(const MachineInstr &MI);
 
   /// Does this instruction rename a GPR without modifying bits?
   static bool isGPRCopy(const MachineInstr &MI);
 
   /// Does this instruction rename an FPR without modifying bits?
   static bool isFPRCopy(const MachineInstr &MI);
 
-  /// Return true if this is load/store scales or extends its register offset.
-  /// This refers to scaling a dynamic index as opposed to scaled immediates.
-  /// MI should be a memory op that allows scaled addressing.
-  static bool isScaledAddr(const MachineInstr &MI);
-
   /// Return true if pairing the given load or store is hinted to be
   /// unprofitable.
   static bool isLdStPairSuppressed(const MachineInstr &MI);
 
   /// Return true if the given load or store is a strided memory access.
   static bool isStridedAccess(const MachineInstr &MI);
 
   /// Return true if this is an unscaled load/store.
public:
   static bool isFalkorShiftExtFast(const MachineInstr &MI);
   /// Returns true if the instruction is a long operation using the upper half
   /// of vectors.
   static bool isLongVectorUpperHalf(const MachineInstr &MI);
   /// Return true if the instructions is a SEH instruciton used for unwinding
   /// on Windows.
   static bool isSEHInstruction(const MachineInstr &MI);
 
+#define GET_TII_HELPER_DECLS
+#include "AArch64GenInstrInfo.inc"
+
 private:
   /// Sets the offsets on outlined instructions in \p MBB which use SP
   /// so that they will be valid post-outlining.
   ///
   /// \param MBB A \p MachineBasicBlock in an outlined function.
   void fixupPostOutline(MachineBasicBlock &MBB) const;
 
   void instantiateCondBranch(MachineBasicBlock &MBB, const DebugLoc &DL,

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

if (MI.getOperand(0).getSubReg() == 0 && MI.getOperand(1).isFI() &&
       FrameIndex = MI.getOperand(1).getIndex();
       return MI.getOperand(0).getReg();
     }
     break;
   }
   return 0;
 }
 
-/// Return true if this is load/store scales or extends its register offset.
-/// This refers to scaling a dynamic index as opposed to scaled immediates.
-/// MI should be a memory op that allows scaled addressing.
-bool AArch64InstrInfo::isScaledAddr(const MachineInstr &MI) {
-  switch (MI.getOpcode()) {
-  default:
-    break;
-  case AArch64::LDRBBroW:
-  case AArch64::LDRBroW:
-  case AArch64::LDRDroW:
-  case AArch64::LDRHHroW:
-  case AArch64::LDRHroW:
-  case AArch64::LDRQroW:
-  case AArch64::LDRSBWroW:
-  case AArch64::LDRSBXroW:
-  case AArch64::LDRSHWroW:
-  case AArch64::LDRSHXroW:
-  case AArch64::LDRSWroW:
-  case AArch64::LDRSroW:
-  case AArch64::LDRWroW:
-  case AArch64::LDRXroW:
-  case AArch64::STRBBroW:
-  case AArch64::STRBroW:
-  case AArch64::STRDroW:
-  case AArch64::STRHHroW:
-  case AArch64::STRHroW:
-  case AArch64::STRQroW:
-  case AArch64::STRSroW:
-  case AArch64::STRWroW:
-  case AArch64::STRXroW:
-  case AArch64::LDRBBroX:
-  case AArch64::LDRBroX:
-  case AArch64::LDRDroX:
-  case AArch64::LDRHHroX:
-  case AArch64::LDRHroX:
-  case AArch64::LDRQroX:
-  case AArch64::LDRSBWroX:
-  case AArch64::LDRSBXroX:
-  case AArch64::LDRSHWroX:
-  case AArch64::LDRSHXroX:
-  case AArch64::LDRSWroX:
-  case AArch64::LDRSroX:
-  case AArch64::LDRWroX:
-  case AArch64::LDRXroX:
-  case AArch64::STRBBroX:
-  case AArch64::STRBroX:
-  case AArch64::STRDroX:
-  case AArch64::STRHHroX:
-  case AArch64::STRHroX:
-  case AArch64::STRQroX:
-  case AArch64::STRSroX:
-  case AArch64::STRWroX:
-  case AArch64::STRXroX:
-
-    unsigned Val = MI.getOperand(3).getImm();
-    AArch64_AM::ShiftExtendType ExtType = AArch64_AM::getMemExtendType(Val);
-    return (ExtType != AArch64_AM::UXTX) || AArch64_AM::getMemDoShift(Val);
-  }
-  return false;
-}
-
 /// Check all MachineMemOperands for a hint to suppress pairing.
 bool AArch64InstrInfo::isLdStPairSuppressed(const MachineInstr &MI) {
   return llvm::any_of(MI.memoperands(), [](MachineMemOperand *MMO) {
     return MMO->getFlags() & MOSuppressPair;
   });
 }
 
 /// Set a flag on the first MachineMemOperand to suppress pairing.
MachineBasicBlock::iterator AArch64InstrInfo::insertOutlinedCall(
   It = MBB.insert(It, Restore);
   return CallPt;
 }
 
 bool AArch64InstrInfo::shouldOutlineFromFunctionByDefault(
   MachineFunction &MF) const {
   return MF.getFunction().optForMinSize();
 }
+
+#define GET_TII_HELPERS
+#include "AArch64GenInstrInfo.inc"

llvm/lib/Target/AArch64/AArch64SchedPredicates.td

+//===- AArch64SchedPredicates.td - AArch64 Sched Preds -----*- tablegen -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines scheduling predicate definitions that are used by the
+// AArch64 subtargets.
+//
+//===----------------------------------------------------------------------===//
+
+// Function mappers.
+
+// Check the extension type in the register offset addressing mode.
+let FunctionMapper = "AArch64_AM::getMemExtendType" in {
+  def CheckMemExtUXTW : CheckImmOperand_s<3, "AArch64_AM::UXTW">;
+  def CheckMemExtLSL  : CheckImmOperand_s<3, "AArch64_AM::UXTX">;
+  def CheckMemExtSXTW : CheckImmOperand_s<3, "AArch64_AM::SXTW">;
+  def CheckMemExtSXTX : CheckImmOperand_s<3, "AArch64_AM::SXTX">;
+}
+
+// Check for scaling in the register offset addressing mode.
+let FunctionMapper = "AArch64_AM::getMemDoShift" in
+def CheckMemScaled  : CheckImmOperandSimple<3>;
+
+// Generic predicates.
+
+// Identify whether an instruction is a load
+// using the register offset addressing mode.
+def IsLoadRegOffsetPred  : CheckOpcode<[PRFMroW, PRFMroX,
+                                        LDRBBroW, LDRBBroX,
+                                        LDRSBWroW, LDRSBWroX, LDRSBXroW, LDRSBXroX,
+                                        LDRHHroW, LDRHHroX,
+                                        LDRSHWroW, LDRSHWroX, LDRSHXroW, LDRSHXroX,
+                                        LDRWroW, LDRWroX,
+                                        LDRSWroW, LDRSWroX,
+                                        LDRXroW, LDRXroX,
+                                        LDRBroW, LDRBroX,
+                                        LDRHroW, LDRHroX,
+                                        LDRSroW, LDRSroX,
+                                        LDRDroW, LDRDroX]>;
+
+// Identify whether an instruction is a load
+// using the register offset addressing mode.
+def IsStoreRegOffsetPred : CheckOpcode<[STRBBroW, STRBBroX,
+                                        STRHHroW, STRHHroX,
+                                        STRWroW, STRWroX,
+                                        STRXroW, STRXroX,
+                                        STRBroW, STRBroX,
+                                        STRHroW, STRHroX,
+                                        STRSroW, STRSroX,
+                                        STRDroW, STRDroX]>;
+
+// Target predicates.
+
+// Identify a load or store using the register offset addressing mode
+// with an extended or scaled register.
+def ScaledIdxFn   : TIIPredicate<"isScaledAddr",
+                                 MCOpcodeSwitchStatement<
+                                   [MCOpcodeSwitchCase<
+                                      !listconcat(IsLoadRegOffsetPred.ValidOpcodes,
+                                                  IsStoreRegOffsetPred.ValidOpcodes),
+                                      MCReturnStatement<
+                                        CheckAny<[CheckNot<CheckMemExtLSL>,
+                                                  CheckMemScaled]>>>],
+                                   MCReturnStatement<FalsePred>>>;
+def ScaledIdxPred : MCSchedPredicate<ScaledIdxFn>;

llvm/lib/Target/AArch64/AArch64Schedule.td

 def ReadAdrBase : SchedRead; // Read the base resister of a reg-offset LD/ST.
 
 // Predicate for determining when a shiftable register is shifted.
 def RegShiftedPred : SchedPredicate<[{TII->hasShiftedReg(*MI)}]>;
 
 // Predicate for determining when a extendedable register is extended.
 def RegExtendedPred : SchedPredicate<[{TII->hasExtendedReg(*MI)}]>;
 
-// ScaledIdxPred is true if a WriteLDIdx operand will be
-// scaled. Subtargets can use this to dynamically select resources and
-// latency for WriteLDIdx and ReadAdrBase.
-def ScaledIdxPred : SchedPredicate<[{TII->isScaledAddr(*MI)}]>;
-
 // Serialized two-level address load.
 // EXAMPLE: LOADGot
 def WriteLDAdr : WriteSequence<[WriteAdr, WriteLD]>;
 
 // Serialized two-level address lookup.
 // EXAMPLE: MOVaddr...
 def WriteAdrAdr : WriteSequence<[WriteAdr, WriteAdr]>;
 

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

 //===-- AArch64Subtarget.cpp - AArch64 Subtarget Information ----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the AArch64 specific subclass of TargetSubtarget.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AArch64Subtarget.h"
 
 #include "AArch64.h"
-#include "AArch64InstrInfo.h"
-#include "AArch64PBQPRegAlloc.h"
-#include "AArch64TargetMachine.h"
-
 #include "AArch64CallLowering.h"
+#include "AArch64InstrInfo.h"
 #include "AArch64LegalizerInfo.h"
+#include "AArch64PBQPRegAlloc.h"
 #include "AArch64RegisterBankInfo.h"
+#include "AArch64TargetMachine.h"
+#include "MCTargetDesc/AArch64AddressingModes.h"
 #include "llvm/CodeGen/GlobalISel/InstructionSelect.h"
 #include "llvm/CodeGen/MachineScheduler.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Support/TargetParser.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "aarch64-subtarget"

llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.h

 // register name to register number.
 //
 #define GET_REGINFO_ENUM
 #include "AArch64GenRegisterInfo.inc"
 
 // Defines symbolic names for the AArch64 instructions.
 //
 #define GET_INSTRINFO_ENUM
+#define GET_GENINSTRINFO_MC_DECL
 #include "AArch64GenInstrInfo.inc"
 
 #define GET_SUBTARGETINFO_ENUM
 #include "AArch64GenSubtargetInfo.inc"
 
 #endif

llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "AArch64MCTargetDesc.h"
 #include "AArch64ELFStreamer.h"
 #include "AArch64MCAsmInfo.h"
 #include "AArch64WinCOFFStreamer.h"
 #include "InstPrinter/AArch64InstPrinter.h"
+#include "MCTargetDesc/AArch64AddressingModes.h"
 #include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCInstrAnalysis.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetRegistry.h"
 
 using namespace llvm;
 
 #define GET_INSTRINFO_MC_DESC
+#define GET_GENINSTRINFO_MC_HELPERS
 #include "AArch64GenInstrInfo.inc"
 
 #define GET_SUBTARGETINFO_MC_DESC
 #include "AArch64GenSubtargetInfo.inc"
 
 #define GET_REGINFO_MC_DESC
 #include "AArch64GenRegisterInfo.inc"
 

llvm/test/tools/llvm-mca/AArch64/Cyclone/register-offset.s

 # NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py
 # RUN: llvm-mca -march=aarch64 -mcpu=cyclone -resource-pressure=false < %s | FileCheck %s
 
   ldr x7, [x1, #8]
   ldr x6, [x1, x2]
   ldr  x4, [x1, x2, sxtx]
 
 # CHECK:      Iterations:        100
 # CHECK-NEXT: Instructions:      300
-# CHECK-NEXT: Total Cycles:      156
-# CHECK-NEXT: Total uOps:        300
+# CHECK-NEXT: Total Cycles:      157
+# CHECK-NEXT: Total uOps:        500
 
 # CHECK:      Dispatch Width:    6
-# CHECK-NEXT: uOps Per Cycle:    1.92
-# CHECK-NEXT: IPC:               1.92
+# CHECK-NEXT: uOps Per Cycle:    3.18
+# CHECK-NEXT: IPC:               1.91
 # CHECK-NEXT: Block RThroughput: 1.5
 
 # CHECK:      Instruction Info:
 # CHECK-NEXT: [1]: #uOps
 # CHECK-NEXT: [2]: Latency
 # CHECK-NEXT: [3]: RThroughput
 # CHECK-NEXT: [4]: MayLoad
 # CHECK-NEXT: [5]: MayStore
 # CHECK-NEXT: [6]: HasSideEffects (U)
 
 # CHECK:      [1]    [2]    [3]    [4]    [5]    [6]    Instructions:
 # CHECK-NEXT:  1      4     0.50    *                   ldr x7, [x1, #8]
-# CHECK-NEXT:  1      4     0.50    *                   ldr x6, [x1, x2]
-# CHECK-NEXT:  1      4     0.50    *                   ldr x4, [x1, x2, sxtx]
+# CHECK-NEXT:  2      5     0.50    *                   ldr x6, [x1, x2]
+# CHECK-NEXT:  2      5     0.50    *                   ldr x4, [x1, x2, sxtx]