[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

   bool isLdstScaledRegNotPlusLsl2(const MachineInstr &MI, unsigned Op) const;
   // Minus reg for ldstso addr mode
   bool isLdstSoMinusReg(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.
   ///

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

   return ARM_AM::getAM2Op(OffImm) == ARM_AM::sub;
 }
 
-// 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;

llvm/lib/Target/ARM/ARMScheduleA57.td

 def IsLdstsoMinusRegPredX2 :
   SchedPredicate<[{TII->isLdstSoMinusReg(*MI, 2)}]>;
 
-// Load, scaled register offset
-def IsLdrAm2ScaledPred :
-  SchedPredicate<[{TII->isAm2ScaledReg(*MI, 1)}]>;
-
 // LDM, base reg in list
 def IsLDMBaseRegInListPred : MCSchedPredicate<IsLDMBaseRegInList>;
 

   "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"

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

 # 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  r8, r9, [r2, #15]
 # CHECK-NEXT:  4      5     2.00    *                   ldrd  r2, r3, [r9, #32]!

 
 # CHECK:      Resource pressure per iteration:
 # CHECK-NEXT: [0]    [1.0]  [1.1]  [2]    [3]    [4]    [5]    [6]
-# CHECK-NEXT:  -     63.00  63.00  160.00 9.00   55.00   -      -
+# CHECK-NEXT:  -     63.00  63.00  160.00 10.00  55.00   -      -
 
 # CHECK:      Resource pressure by instruction:
 # CHECK-NEXT: [0]    [1.0]  [1.1]  [2]    [3]    [4]    [5]    [6]    Instructions:

 # 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  r8, r9, [r2, #15]
 # CHECK-NEXT:  -     1.00   1.00   2.00    -      -      -      -     ldrd  r2, r3, [r9, #32]!