[PATCH] D157458: [X86][AMX] Fix virtual register traversing in case of GlobalIsel

Thu Aug 10 17:09:39 PDT 2023

e-kud added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/MachineRegisterInfo.h:677
   /// None of this condition is possible without GlobalISel for now.
-  /// In other words, if GlobalISel is not used or if the query happens after
+  /// In other words, if GlobalISel is not used and if the query happens after
   /// the select pass, using getRegClass is safe.
----------------
qcolombet wrote:
> pengfei wrote:
> > pengfei wrote:
> > > My understanding the `or` is correct, it looks to me like `if (!GlobalISel || AfterISel)`
> > Oh, I think and makes sense here. Sorry..
> The sentence was correct :).
> What this means is either :
> - `GlobalISel` is not used, i.e., we go through `SelectionDAG` and then when we hit `MachineInstr` then `getRegClass` does what you want, or
> - `GlobalISel` is used but  when you went all the way through the `InstructionSelection` pass in GlobalISel, then using `getRegClass` is also safe.
Yeah, but the second bullet is not correct now. It is not safe to use `getRegClass` after `GlobalISel` because after `InstructionSelection` pass we may have registers without a class. That's why I changed that we shouldn't select using `GlobalISel` at all and we shouldn't be in a selection process (by SelectionDAG or FastISel). Actually I'm not sure whether it is designed so or not.

================
Comment at: llvm/test/CodeGen/X86/AMX/amx-fastpreconfig-gisel.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
+# RUN: llc -mtriple=x86_64-- -run-pass=fastpretileconfig -o - %s | FileCheck %s
+
----------------
qcolombet wrote:
> 1. Unless you pass `-global-isel=1` GlobalISel doesn't run I believe for X86.
> 2. X86FastPreTileConfig runs only at O0. My guess is you're seeing an issue with FastISel.
> 
> Could you do a print-after-all with your failure (starting back at LLVM IR level) and see where the regclass is not properly set?
> It doesn't look like a GlobalISel issue to me.
> Put differently, you shouldn't see any null regclass after ISel (GISel or SDISel) period.
We don't need to pass `-global-isel=1` because this `MIR` is already obtained from `GlobalISel` and `llc` just runs the problematic pass.

On [the previous iteration](https://reviews.llvm.org/D157458?id=548410) I put a test from `.ll` but I think it is not convenient to keep it actual. After specific changes in `GlobalISel` we need to update it and in the worst case we have to find another test when a register doesn't have an assigned class. MIR test solves this problem.

Here is print-after-all output
```
*** IR Dump After Module Verifier (verify) ***
define i64 @f(i64 %0, i64 %1) {
entry:
  %2 = lshr i64 %0, %1
  %3 = add i64 %2, 123456789
  ret i64 %3
}
# *** IR Dump After IRTranslator (irtranslator) ***:
# Machine code for function f: IsSSA, TracksLiveness
Function Live Ins: $rdi, $rsi

bb.1.entry:
  liveins: $rdi, $rsi
  %0:_(s64) = COPY $rdi
  %1:_(s64) = COPY $rsi
  %3:_(s64) = G_CONSTANT i64 123456789
  %2:_(s64) = G_LSHR %0:_, %1:_(s64)
  %4:_(s64) = G_ADD %2:_, %3:_
  $rax = COPY %4:_(s64)
  RET 0, implicit $rax

# End machine code for function f.

# *** IR Dump After Legalizer (legalizer) ***:
# Machine code for function f: IsSSA, TracksLiveness, Legalized
Function Live Ins: $rdi, $rsi

bb.1.entry:
  liveins: $rdi, $rsi
  %0:_(s64) = COPY $rdi
  %1:_(s64) = COPY $rsi
  %3:_(s64) = G_CONSTANT i64 123456789
  %5:_(s8) = G_TRUNC %1:_(s64)
  %2:_(s64) = G_LSHR %0:_, %5:_(s8)
  %4:_(s64) = G_ADD %2:_, %3:_
  $rax = COPY %4:_(s64)
  RET 0, implicit $rax

# End machine code for function f.

# *** IR Dump After RegBankSelect (regbankselect) ***:
# Machine code for function f: IsSSA, TracksLiveness, Legalized, RegBankSelected
Function Live Ins: $rdi, $rsi

bb.1.entry:
  liveins: $rdi, $rsi
  %0:gpr(s64) = COPY $rdi
  %1:gpr(s64) = COPY $rsi
  %3:gpr(s64) = G_CONSTANT i64 123456789
  %5:gpr(s8) = G_TRUNC %1:gpr(s64)
  %2:gpr(s64) = G_LSHR %0:gpr, %5:gpr(s8)
  %4:gpr(s64) = G_ADD %2:gpr, %3:gpr
  $rax = COPY %4:gpr(s64)
  RET 0, implicit $rax

# End machine code for function f.

# *** IR Dump After InstructionSelect (instruction-select) ***:
# Machine code for function f: IsSSA, TracksLiveness, Legalized, RegBankSelected, Selected
Function Live Ins: $rdi, $rsi

bb.1.entry:
  liveins: $rdi, $rsi
  %0:gr64 = COPY $rdi
  %1:gr64_with_sub_8bit = COPY $rsi
  %5:gr8 = COPY %1.sub_8bit:gr64_with_sub_8bit
  $cl = COPY %5:gr8
  %2:gr64 = SHR64rCL %0:gr64(tied-def 0), implicit-def $eflags, implicit $cl
  %4:gr64 = ADD64ri32 %2:gr64(tied-def 0), 123456789, implicit-def $eflags
  $rax = COPY %4:gr64
  RET 0, implicit $rax

# End machine code for function f.

# *** IR Dump After Finalize ISel and expand pseudo-instructions (finalize-isel) ***:
# Machine code for function f: IsSSA, TracksLiveness, Legalized, RegBankSelected, Selected
Function Live Ins: $rdi, $rsi

bb.1.entry:
  liveins: $rdi, $rsi
  %0:gr64 = COPY $rdi
  %1:gr64_with_sub_8bit = COPY $rsi
  %5:gr8 = COPY %1.sub_8bit:gr64_with_sub_8bit
  $cl = COPY %5:gr8
  %2:gr64 = SHR64rCL %0:gr64(tied-def 0), implicit-def $eflags, implicit $cl
  %4:gr64 = ADD64ri32 %2:gr64(tied-def 0), 123456789, implicit-def $eflags
  $rax = COPY %4:gr64
  RET 0, implicit $rax
```

A virtual register `%3` is absent after instruction-select and doesn't have an assigned register class, only a register bank.
A comment obtained using debug-only:
```
Selecting:
  %3:gpr(s64) = G_CONSTANT i64 123456789
Is dead; erasing.
```

The test is obtained using `-stop-after`, so you can see that `%3` really has only a register bank not a class. The same I see on `AArch64`. Either both targets do something wrong or the problem is in general GISel algorithm. But we do have null regclass after GISel.

================
Comment at: llvm/test/CodeGen/X86/AMX/amx-fastpreconfig-gisel.mir:4
+
+# GlobalIsel doesn't use all virtual registers and there may be virtual
+# registers without class.
----------------
pengfei wrote:
> How can I check it's GlobalIsel MIR? I didn't find anything special in the test.
Yes, I've put a note
> Note that %3 doesn't have a class.

`  - { id: 3, class: gpr, preferred-register: '' }`
So, it is a register without an assigned class, it has only a `RegisterBank` – `gpr` but not a class. When others has `gr64`, `gr8` and so on.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157458