[ARM][SchedModels] Get rid of IsLdrAm2ScaledPred

[andreadb]

Could you please add more context to the diff? I don't remember how IsLdstsoScaledPredX2 was defined.

[evgeny777]

Uploaded diff with full context

[dmgreen]

Should it always have been checking operand 4 then? I think that makes sense

(More of a commit message might make it more obvious why changes are being made ;) )

[andreadb]

In D90024#2350389, @dmgreen wrote:

Should it always have been checking operand 4 then? I think that makes sense

(More of a commit message might make it more obvious why changes are being made ;) )

I was about to ask something similar...
The original predicate is checking operand 3, not 4. I am a bit confused.

[evgeny777]

Should it always have been checking operand 4 then? I think that makes sense

IMO, it was incorrectly checking for operand 3. I've got an assertion when switched to MC pred

[dmgreen]

LGTM then. Cheers

[andreadb]

In D90024#2350392, @evgeny777 wrote:

Should it always have been checking operand 4 then? I think that makes sense

IMO, it was incorrectly checking for operand 3. I've got an assertion when switched to MC pred

(paranoia level).
Are we sure that the lowering of MachineInstr to MCInst is preserving the operand sequence? Can it be that the immediate is at position 4 for the MCInst only?
I have no idea how an ldrbt looks like as a MachineInstr. The original check should have triggered an assertion too for MachineInstr then...

[dmgreen]

(paranoia level).
Are we sure that the lowering of MachineInstr to MCInst is preserving the operand sequence? Can it be that the immediate is at position 4 for the MCInst only?
I have no idea how an ldrbt looks like as a MachineInstr. The original check should have triggered an assertion too for MachineInstr then...

I was not aware that was a thing that could happen!

I'm unsure if anything would ever generate an ldrbt with negative postinc reg and shift from codegen, so it might be difficult to test. And may never have been tested in the past.

[andreadb]

In D90024#2350536, @dmgreen wrote:

(paranoia level).
Are we sure that the lowering of MachineInstr to MCInst is preserving the operand sequence? Can it be that the immediate is at position 4 for the MCInst only?
I have no idea how an ldrbt looks like as a MachineInstr. The original check should have triggered an assertion too for MachineInstr then...

I was not aware that was a thing that could happen!

I'm unsure if anything would ever generate an ldrbt with negative postinc reg and shift from codegen, so it might be difficult to test. And may never have been tested in the past.

I guess the answer is in llvm::LowerARMMachineInstrToMCInst.

On x86 for example the lowering of the operands is always "trivial" (meaning that the ordering of the machine operands in the MachineInstr is still preserved in the final MCInst). It is basically a foreach loop that produces an MCOperand for every MachineOperand. No operands change in position nor are added/removed. Otherwise, it is unsafe to use MCInstrDesc to describe MCInst too (MCInstDesc is designed for MachineInstr).

I suggest to double-check that it is fine for this particular case. It may well be that the ldrbt is a pseudo, and that the rule was never really triggered in practice.
Otherwise it is not safe to use a MCSchedPredicate for this patch unless we provide a way to map machine operand indices to valid MCOperand indices (only for those instructions for which lowering is not trivial).

[evgeny777]

(paranoia level).
Are we sure that the lowering of MachineInstr to MCInst is preserving the operand sequence? Can it be that the immediate is at position 4 for the MCInst only?
I have no idea how an ldrbt looks like as a MachineInstr. The original check should have triggered an assertion too for MachineInstr then...

@andreadb Well, it's interesting thought and I, honestly, do not know. However with LDRBT it doesn't seem to be the case. Here is handwritten MIR:

name:            test_ldrbt
tracksRegLiveness: true
body: |
  bb.0:
    liveins: $r1, $r3, $r4
    %1:gprnopc = COPY $r1
    %3:gprnopc = COPY $r3
    %4:gprnopc = COPY $r4
    %3:gprnopc, %4:gprnopc = LDRBT_POST_REG %4:gprnopc, %1:gprnopc, 20492, 0, $noreg

    $r0 = COPY %3:gprnopc
    BX_RET 14, $noreg, implicit $r0

If you're interested you can try transforming it to asm with llc -start-before=greedy -filetype=asm ... and check operand sequence in assembly with llvm-mc --debug

[andreadb]

So, it doesn't look like the lowering is doing anything odd with the operand sequence. It should be fine.
At this point, the only explanation is that the original predicate was never really executed for that write variant. So it was never tested. I don't see other alternatives honestly.

[dmgreen]

I'm happy with that explanation, and would be happy with this patch going ahead. The mir test above seems to show it failing before and working now.

[andreadb]

In D90024#2350657, @dmgreen wrote:

I'm happy with that explanation, and would be happy with this patch going ahead. The mir test above seems to show it failing before and working now.

Exactly:

llc: /home/andrea/llvm-project/llvm/include/llvm/CodeGen/MachineOperand.h:536: int64_t llvm::MachineOperand::getImm() const: Assertion `isImm() && "Wrong MachineOperand accessor"' failed.

It looks like that predicate has always been wrong.
Sorry for causing panic :-P (I guess, better safe than sorry).

llvm/lib/Target/ARM/ARMBaseInstrInfo.h

public:
 
   bool isPredicable(const MachineInstr &MI) const override;
 
   // CPSR defined in instruction
   static bool isCPSRDefined(const MachineInstr &MI);
 
   // Load, scaled register offset, not plus LSL2
   bool isLdstScaledRegNotPlusLsl2(const MachineInstr &MI, unsigned Op) const;
-  // Scaled register offset in address mode 2
-  bool isAm2ScaledReg(const MachineInstr &MI, unsigned Op) const;
 
   /// GetInstSize - Returns the size of the specified MachineInstr.
   ///
   unsigned getInstSizeInBytes(const MachineInstr &MI) const override;
 
   unsigned isLoadFromStackSlot(const MachineInstr &MI,
                                int &FrameIndex) const override;
   unsigned isStoreToStackSlot(const MachineInstr &MI,

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

bool ARMBaseInstrInfo::isLdstScaledRegNotPlusLsl2(const MachineInstr &MI,
   bool isAdd = ARM_AM::getAM2Op(OffImm) == ARM_AM::add;
   unsigned Amt = ARM_AM::getAM2Offset(OffImm);
   ARM_AM::ShiftOpc ShiftOpc = ARM_AM::getAM2ShiftOpc(OffImm);
   if (ShiftOpc == ARM_AM::no_shift) return false; // not scaled
   bool SimpleScaled = (isAdd && ShiftOpc == ARM_AM::lsl && Amt == 2);
   return !SimpleScaled;
 }
 
-// Load, scaled register offset
-bool ARMBaseInstrInfo::isAm2ScaledReg(const MachineInstr &MI,
-                                      unsigned Op) const {
-  unsigned OffImm = MI.getOperand(Op + 2).getImm();
-  return ARM_AM::getAM2ShiftOpc(OffImm) != ARM_AM::no_shift;
-}
-
 static bool isEligibleForITBlock(const MachineInstr *MI) {
   switch (MI->getOpcode()) {
   default: return true;
   case ARM::tADC:   // ADC (register) T1
   case ARM::tADDi3: // ADD (immediate) T1
   case ARM::tADDi8: // ADD (immediate) T2
   case ARM::tADDrr: // ADD (register) T1
   case ARM::tAND:   // AND (register) T1

llvm/lib/Target/ARM/ARMScheduleA57.td

def IsLdstsoScaledNotOptimalPredX2 :
   SchedPredicate<[{TII->isLdstScaledRegNotPlusLsl2(*MI, 2)}]>;
 
 def IsLdstsoScaledPredX2 : MCSchedPredicate<CheckNot<CheckExtNoShift>>;
 
 def IsLdstsoMinusRegPredX0 : MCSchedPredicate<CheckAM2OpSub<2>>;
 def IsLdstsoMinusRegPred : MCSchedPredicate<CheckAM2OpSub<3>>;
 def IsLdstsoMinusRegPredX2 : MCSchedPredicate<CheckAM2OpSub<4>>;
 
-// Load, scaled register offset
-def IsLdrAm2ScaledPred :
-  SchedPredicate<[{TII->isAm2ScaledReg(*MI, 1)}]>;
-
 // LDM, base reg in list
 def IsLDMBaseRegInListPred : MCSchedPredicate<IsLDMBaseRegInList>;
 
 class A57WriteLMOpsListType<list<SchedWriteRes> writes> {
   list <SchedWriteRes> Writes = writes;
   SchedMachineModel SchedModel = ?;
 }
 
def : InstRW<[A57Write_4cyc_1L_1I, A57WriteLdrAm3PostWrBack],
   (instregex "LDR(H|SH|SB)_POST")>;
 def : InstRW<[A57Write_4cyc_1L, A57WrBackOne],
   (instregex "t2LDR(H|SH|SB)?_POST")>;
 
 def : InstRW<[A57Write_4cyc_1L_1I, A57WrBackTwo], (instregex "LDR_POST_REG",
   "LDRB_POST_REG", "LDR(B?)T_POST$")>;
 
 def A57WriteLdrTRegPost : SchedWriteVariant<[
-  SchedVar<IsLdrAm2ScaledPred, [A57Write_4cyc_1I_1L_1M]>,
+  SchedVar<IsLdstsoScaledPredX2, [A57Write_4cyc_1I_1L_1M]>,
   SchedVar<NoSchedPred,        [A57Write_4cyc_1L_1I]>
 ]>;
 def A57WriteLdrTRegPostWrBack : SchedWriteVariant<[
-  SchedVar<IsLdrAm2ScaledPred, [A57WrBackThree]>,
+  SchedVar<IsLdstsoScaledPredX2, [A57WrBackThree]>,
   SchedVar<NoSchedPred,        [A57WrBackTwo]>
 ]>;
 // 4(3) "I0/I1,L,M" for scaled register, otherwise 4(2) "I0/I1,L"
 def : InstRW<[A57WriteLdrTRegPost, A57WriteLdrTRegPostWrBack],
   (instregex "LDRT_POST_REG", "LDRBT_POST_REG")>;
 
 def : InstRW<[A57Write_4cyc_1L_1I, A57WrBackTwo], (instregex "LDR(SB|H|SH)Tr")>;
 

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

 # CHECK-NEXT:  2      4     1.00    *                   ldrb  r6, [r9, -r3]!
 # CHECK-NEXT:  2      4     1.00    *                   ldrb  r2, [r1], r4
 # CHECK-NEXT:  2      4     1.00    *                   ldrb  r8, [r4], -r5
 # CHECK-NEXT:  2      5     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:  3      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  r0, r1, [r5, r2]
 # CHECK-NEXT:  4      5     2.00    *                   ldrd  r0, r1, [r5, -r2]
 # CHECK-NEXT:  2      4     2.00    *                   ldrd  r8, r9, [r2, #15]
 # 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: [2]   - A57UnitL
 # CHECK-NEXT: [3]   - A57UnitM
 # CHECK-NEXT: [4]   - A57UnitS
 # CHECK-NEXT: [5]   - A57UnitW
 # CHECK-NEXT: [6]   - A57UnitX
 
 # CHECK:      Resource pressure per iteration:
 # CHECK-NEXT: [0]    [1.0]  [1.1]  [2]    [3]    [4]    [5]    [6]
-# CHECK-NEXT:  -     69.00  69.00  167.00 9.00   57.00   -      -
+# CHECK-NEXT:  -     69.00  69.00  167.00 10.00  57.00   -      -
 
 # CHECK:      Resource pressure by instruction:
 # CHECK-NEXT: [0]    [1.0]  [1.1]  [2]    [3]    [4]    [5]    [6]    Instructions:
 # CHECK-NEXT:  -      -      -     1.00    -      -      -      -     pld [pc, #8]
 # CHECK-NEXT:  -      -      -     1.00    -      -      -      -     pldw  [pc, #-128]
 # CHECK-NEXT:  -      -      -     1.00    -      -      -      -     ldr r5, [r7]
 # CHECK-NEXT:  -      -      -     1.00    -      -      -      -     ldr r6, [r3, #63]
 # CHECK-NEXT:  -     0.50   0.50   1.00    -      -      -      -     ldr r2, [r4, #4095]!

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