[PATCH] D152059: [AVR] Replace shift-to-loop IR pass with common shift code

Sat Jun 17 04:16:16 PDT 2023

Patryk27 abandoned this revision.
Patryk27 marked 11 inline comments as done.
Patryk27 added a comment.

I've addressed all the comments and I'll create a new revision with them in a moment :-)

================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:2219
+  // Shift the registers by one.
+  insertMultibyteShift(LoopBB->end(), LoopBB, dl, PhiRegPairs, Opc, 1);
+
----------------
benshi001 wrote:
> Will the field `second` of `PhiRegPairs` be over written to `sub_lo` or `sub_hi` by `insertMultibyteShift` ?
I'm not sure what you mean 🤔

`sub_lo` and `sub_hi` are used in `insertWideShift` which is not called here - and `insertMultibyteShift`, given `ShiftAmt = 1` (like in here), reduces down to this block:

```
while (ShiftLeft && ShiftAmt) {
  // Shift one to the left.
  for (ssize_t I = Regs.size() - 1; I >= 0; I--) {
    Register Out = MRI.createVirtualRegister(&AVR::GPR8RegClass);
    Register In = Regs[I].first;
    Register InSubreg = Regs[I].second;
    if (I == (ssize_t)Regs.size() - 1) { // first iteration
      BuildMI(*BB, MBBI, DL, TII.get(AVR::ADDRdRr), Out)
          .addReg(In, 0, InSubreg)
          .addReg(In, 0, InSubreg);
    } else {
      BuildMI(*BB, MBBI, DL, TII.get(AVR::ADCRdRr), Out)
          .addReg(In, 0, InSubreg)
          .addReg(In, 0, InSubreg);
    }
    Regs[I] = std::pair(Out, 0);
  }
  ShiftAmt--;
}
```

... which does overwrite `PhiRegPairs` with the output register.

================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:2229
+        .addMBB(LoopBB);
+    Regs[I] = std::pair(PhiRegs[I], 0);
+  }
----------------
benshi001 wrote:
> After this line, how is the field `second` of `Regs[I]` set to `sub_lo` or `sub_hi`? Would it be better to
> 
> ```
> Regs[I] = PhiRegPairs[I];
> ```
> 
> ?
Note that `insertMultibyteShift` modifies given `Regs` - it changes them from input-registers into output-registers (see the comment above with the related code block), and so doing:

```
Regs[I] = PhiRegPairs[I];
```

... is different from doing:

```
Regs[I] = std::pair(PhiRegs[I], 0);
```

... in the sense that `PhiRegPairs` there contains output-registers (with shifted values), while `PhiRegs` continues to refer to the input-registers, so both fragments do something else.

================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:2246
+
+  return ExitBB;
+}
----------------
benshi001 wrote:
> Don't we need to erase the 32-bit shift pseudo instruction from EntryBB ?
> 
> ```
> ExitBB = EntryBB->splitAt(MI, false);
> ``` 
This already happens at the end of `insertWideShift`:

```
  // Remove the pseudo instruction.
  MI.eraseFromParent();
  return BB;
}
```

================
Comment at: llvm/test/CodeGen/AVR/shift-loop.ll:22
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   [[ADDRdRr:%[0-9]+]]:gpr8 = ADDRdRr %10, %10, implicit-def $sreg
+  ; CHECK-NEXT:   [[ADCRdRr:%[0-9]+]]:gpr8 = ADCRdRr %9, %9, implicit-def $sreg, implicit $sreg
----------------
benshi001 wrote:
> Where is `%10` defined ?
Nice catch - it seems that `update_mir_test_checks.py` cannot resolve cases where declaration happens after usage, i.e. the current approach generates:

```
body:             |
  bb.0 (%ir-block.0):
    successors: %bb.2(0x80000000)
    liveins: $r23r22, $r25r24, $r19r18

    %2:dregs = COPY $r19r18
    %1:dregs = COPY $r25r24
    %0:dregs = COPY $r23r22
    %4:gpr8 = COPY %2.sub_lo
    RJMPk %bb.2

  bb.1 (%ir-block.0):
    successors: %bb.2(0x80000000)

    %12:gpr8 = ADDRdRr %10, %10, implicit-def $sreg
    %13:gpr8 = ADCRdRr %9, %9, implicit-def $sreg, implicit $sreg
    %14:gpr8 = ADCRdRr %8, %8, implicit-def $sreg, implicit $sreg
    %15:gpr8 = ADCRdRr %7, %7, implicit-def $sreg, implicit $sreg

  bb.2 (%ir-block.0):
    successors: %bb.1(0x40000000), %bb.3(0x40000000)

    %7:gpr8 = PHI %1.sub_hi, %bb.0, %15, %bb.1
    %8:gpr8 = PHI %1.sub_lo, %bb.0, %14, %bb.1
    %9:gpr8 = PHI %0.sub_hi, %bb.0, %13, %bb.1
    %10:gpr8 = PHI %0.sub_lo, %bb.0, %12, %bb.1
    %16:gpr8 = PHI %4, %bb.0, %17, %bb.1
    %17:gpr8 = DECRd %16, implicit-def $sreg
    BRPLk %bb.1, implicit $sreg

  bb.3 (%ir-block.0):
    %6:dregs = REG_SEQUENCE %7, %subreg.sub_hi, %8, %subreg.sub_lo
    %5:dregs = REG_SEQUENCE %9, %subreg.sub_hi, %10, %subreg.sub_lo
    $r23r22 = COPY %5
    $r25r24 = COPY %6
    RET implicit $r23r22, implicit $r25r24, implicit $r1
```

To be fair, I'm not sure why those blocks are generated in this order - I switched it now to a simpler approach (note different block order and `BRMIk`):

```
body:             |
  bb.0 (%ir-block.0):
    successors: %bb.1(0x80000000)
    liveins: $r23r22, $r25r24, $r19r18

    %2:dregs = COPY $r19r18
    %1:dregs = COPY $r25r24
    %0:dregs = COPY $r23r22
    %4:gpr8 = COPY %2.sub_lo

  bb.1 (%ir-block.0):
    successors: %bb.2(0x40000000), %bb.3(0x40000000)

    %7:gpr8 = PHI %1.sub_hi, %bb.0, %15, %bb.2
    %8:gpr8 = PHI %1.sub_lo, %bb.0, %14, %bb.2
    %9:gpr8 = PHI %0.sub_hi, %bb.0, %13, %bb.2
    %10:gpr8 = PHI %0.sub_lo, %bb.0, %12, %bb.2
    %16:gpr8 = PHI %4, %bb.0, %17, %bb.2
    %17:gpr8 = DECRd %16, implicit-def $sreg
    BRMIk %bb.3, implicit $sreg

  bb.2 (%ir-block.0):
    successors: %bb.1(0x80000000)

    %12:gpr8 = ADDRdRr %10, %10, implicit-def $sreg
    %13:gpr8 = ADCRdRr %9, %9, implicit-def $sreg, implicit $sreg
    %14:gpr8 = ADCRdRr %8, %8, implicit-def $sreg, implicit $sreg
    %15:gpr8 = ADCRdRr %7, %7, implicit-def $sreg, implicit $sreg
    RJMPk %bb.1

  bb.3 (%ir-block.0):
    %6:dregs = REG_SEQUENCE %7, %subreg.sub_hi, %8, %subreg.sub_lo
    %5:dregs = REG_SEQUENCE %9, %subreg.sub_hi, %10, %subreg.sub_lo
    $r23r22 = COPY %5
    $r25r24 = COPY %6
    RET implicit $r23r22, implicit $r25r24, implicit $r1
```

... and it works correctly (+ generates the same assembly, even), so I guess we'll stick to that.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152059/new/

https://reviews.llvm.org/D152059