[ARM][SchedModels] Convert IsLdrAm3RegOffPred to MCSchedPredicate

[andreadb]

Hi Eugene,

could you please upload a diff with the full context?

Do you know why isAddrMode3OpMinusReg and isAddrMode3OpImm are not declared as static members of ARMBaseInstrInfo?
It seems to me that those should be static. I am asking this question because there may be better ways to implement this patch without having to touch the predicate expander.
Most of the checks performed by those predicates are just checking if operands are registers or specific immediate values. We already have specialised predicates for describing constraints on register operands and immediate operands. I would personally use CheckFunctionPredicate only as a last resort, if there is no other way to express the same predicate.

[evgeny777]

@andreadb Makes sense. Let's try AArch64-like approach for this case. I've updated patch to handle just IsLdrAm3RegOffPred

[andreadb]

llvm/lib/Target/ARM/ARMScheduleA57.td

31–34

It seems to me that the original intent of isAddrMode3OpImm is to check if an operand is NOT the invalid register.

isAddrMode3OpImm also seems to require that the operand must be a register (otherwise the getReg() would have trigger an assertion failure - since isReg() would have returned false).

CheckIsRegOperand(idx) is simply testing if the operand at index idx is a register. However, it doesn't check the actual value of that register.

You should be able to use CheckInvalidRegOperand.
Basically, the original implementation is equivalent to:
CheckNot<CheckInvalidRegOperand<index>>.

That being said, in the original code, the return value from isAddrMode3OpImm is also negated (example):

!TII->isAddrMode3OpImm(*MI, 1)

Assuming that my understanding of the code is correct, I guess that you should be able to just use a simple CheckInvalidRegOperand in your case.

[evgeny777]

llvm/lib/Target/ARM/ARMScheduleA57.td

31–34

There is not operator in the original code {!TII->isAddrMode3OpImm(*MI, 1)} which I forgot about :(. I'll update the diff shortly

[evgeny777]

Addressed

[andreadb]

In D89876#2344978, @evgeny777 wrote:

Addressed

Thanks!

I cannot comment on the mca numbers (I leave that part of the review to @dmgreen).
The changes to the predicates look good to me.

---

As a side note (unrelated to this patch):
If in future you plan to rewrite isAddrMode3OpMinusReg, then I suggest you to have a look at TIIPredicate.
Note also that CheckImmOperand_s can be used to define predicates like this:

MyFunction(MI->getOperand(Index).getImm()) == Val

(see for example the code comments for CheckOperandBase in TargetInstrPredicate.td).

[dmgreen]

Change LGTM. Thanks

llvm/lib/Target/ARM/ARMBaseInstrInfo.h

public:
 
   bool ClobbersPredicate(MachineInstr &MI, std::vector<MachineOperand> &Pred,
                          bool SkipDead) const override;
 
   bool isPredicable(const MachineInstr &MI) const override;
 
   // CPSR defined in instruction
   static bool isCPSRDefined(const MachineInstr &MI);
-  bool isAddrMode3OpImm(const MachineInstr &MI, unsigned Op) const;
   bool isAddrMode3OpMinusReg(const MachineInstr &MI, unsigned Op) const;
 
   // Load, scaled register offset
   bool isLdstScaledReg(const MachineInstr &MI, unsigned Op) const;
   // Load, scaled register offset, not plus LSL2
   bool isLdstScaledRegNotPlusLsl2(const MachineInstr &MI, unsigned Op) const;
   // Minus reg for ldstso addr mode
   bool isLdstSoMinusReg(const MachineInstr &MI, unsigned Op) const;

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

 
 bool ARMBaseInstrInfo::isCPSRDefined(const MachineInstr &MI) {
   for (const auto &MO : MI.operands())
     if (MO.isReg() && MO.getReg() == ARM::CPSR && MO.isDef() && !MO.isDead())
       return true;
   return false;
 }
 
-bool ARMBaseInstrInfo::isAddrMode3OpImm(const MachineInstr &MI,
-                                        unsigned Op) const {
-  const MachineOperand &Offset = MI.getOperand(Op + 1);
-  return Offset.getReg() != 0;
-}
-
 // Load with negative register offset requires additional 1cyc and +I unit
 // for Cortex A57
 bool ARMBaseInstrInfo::isAddrMode3OpMinusReg(const MachineInstr &MI,
                                              unsigned Op) const {
   const MachineOperand &Offset = MI.getOperand(Op + 1);
   const MachineOperand &Opc = MI.getOperand(Op + 2);
   assert(Opc.isImm());
   assert(Offset.isReg());

llvm/lib/Target/ARM/ARMScheduleA57.td

 
 def IsCPSRDefinedAndPredicated : CheckAll<[IsCPSRDefined, IsPredicated]>;
 def IsCPSRDefinedAndPredicatedPred :
     MCSchedPredicate<IsCPSRDefinedAndPredicated>;
 
 // Cortex A57 rev. r1p0 or later (false = r0px)
 def IsR1P0AndLaterPred : SchedPredicate<[{false}]>;
 
-// If Addrmode3 contains register offset (not immediate)
-def IsLdrAm3RegOffPred :
-  SchedPredicate<[{!TII->isAddrMode3OpImm(*MI, 1)}]>;
-// The same predicate with operand offset 2 and 3:
-def IsLdrAm3RegOffPredX2 :
-  SchedPredicate<[{!TII->isAddrMode3OpImm(*MI, 2)}]>;
-def IsLdrAm3RegOffPredX3 :
-  SchedPredicate<[{!TII->isAddrMode3OpImm(*MI, 3)}]>;
+def IsLdrAm3RegOffPred : MCSchedPredicate<CheckInvalidRegOperand<2>>;
+def IsLdrAm3RegOffPredX2 : MCSchedPredicate<CheckInvalidRegOperand<3>>;
+def IsLdrAm3RegOffPredX3 : MCSchedPredicate<CheckInvalidRegOperand<4>>;
 
 // If Addrmode3 contains "minus register"
 def IsLdrAm3NegRegOffPred :
   SchedPredicate<[{TII->isAddrMode3OpMinusReg(*MI, 1)}]>;
 // The same predicate with operand offset 2 and 3:
 def IsLdrAm3NegRegOffPredX2 :
   SchedPredicate<[{TII->isAddrMode3OpMinusReg(*MI, 2)}]>;
 def IsLdrAm3NegRegOffPredX3 :
   SchedPredicate<[{TII->isAddrMode3OpMinusReg(*MI, 3)}]>;

llvm/test/tools/llvm-mca/ARM/cortex-a57-memory-instructions.s

 # CHECK-NEXT:  1      4     1.00    *                   ldrb  r7, [r12, -r1, lsl #15]
 # CHECK-NEXT:  2      4     1.00    *                   ldrb  r5, [r2], r9, asr #15
 # CHECK-NEXT:  2      4     1.00    *                   ldrbt r3, [r1], #4
 # CHECK-NEXT:  2      4     1.00    *                   ldrbt r2, [r8], #-8
 # CHECK-NEXT:  2      4     1.00    *                   ldrbt r8, [r7], r6
 # CHECK-NEXT:  2      4     1.00    *                   ldrbt r1, [r2], -r6, lsl #12
 # CHECK-NEXT:  2      4     2.00    *                   ldrd  r0, r1, [r5]
 # CHECK-NEXT:  2      4     2.00    *                   ldrd  r8, r9, [r2, #15]
-# CHECK-NEXT:  4      4     2.00    *                   ldrd  r2, r3, [r9, #32]!
+# CHECK-NEXT:  4      5     2.00    *                   ldrd  r2, r3, [r9, #32]!
 # CHECK-NEXT:  4      4     2.00    *                   ldrd  r6, r7, [r1], #8
 # CHECK-NEXT:  4      4     2.00    *                   ldrd  r2, r3, [r8], #0
 # CHECK-NEXT:  4      4     2.00    *                   ldrd  r2, r3, [r8], #0
 # CHECK-NEXT:  4      4     2.00    *                   ldrd  r2, r3, [r8], #-0
 # CHECK-NEXT:  2      4     2.00    *                   ldrd  r4, r5, [r1, r3]
 # CHECK-NEXT:  4      4     2.00    *                   ldrd  r4, r5, [r7, r2]!
 # CHECK-NEXT:  4      4     2.00    *                   ldrd  r0, r1, [r8], r12
 # CHECK-NEXT:  4      4     2.00    *                   ldrd  r0, r1, [r8], -r12