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

Fri Aug 11 02:23:12 PDT 2023

qcolombet added inline comments.

================
Comment at: llvm/lib/Target/X86/X86FastPreTileConfig.cpp:672
+    const TargetRegisterClass *RC = MRI->getRegClassOrNull(VirtReg);
+    if (RC && RC->getID() == X86::TILERegClassID) {
       HasVirtTileReg = true;
----------------
I think the proper fix here would be:
```
 if (!MRI->reg_empty(VirtReg) && // <-- only process alive registers
     MRI->getRegClass(VirtReg)->getID() == X86::TILERegClassID) {
```

================
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
+
----------------
pengfei wrote:
> e-kud wrote:
> > e-kud wrote:
> > > 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.
> > Yes, it is pretty simple:
> > Elimination of instructions during instruction-select happens before selection by checking `isTriviallyDead` predicate. Classes are assigned during selection.
> > 
> > https://github.com/llvm/llvm-project/commit/931904d7772b18210d7166ed657176ae91ad11d8
> > 
> > > In some tests, the G_CONSTANT vregs never get constrained to a class:
> > > the only use of the vreg was folded into another instruction, so the
> > > G_CONSTANT, now dead, never gets selected.
> @qcolombet reminds me, X86FastPreTileConfig was intended to solve for FastISel issues. I don't think it can work with GISel.
> 
> It simply checks OptLevel because we just needs to distinguish FastISel from SDISel in the past. The introduction of GISel makes it's invalid. I think we should introduce a method to check the MIR was generated by which one and run this pass only for FastISel.
Ok I understand now.
GISel is correct because the instruction defining the problematic register is dead, i.e., it is not even exposed to GISel selection process.

It cannot happen with SDISel because the registers are instantiated when the actual instructions are produced.

In any case, I believe the fix is wrong: we shouldn't process dead registers. All bets are off on these. E.g., after regalloc they won't have a physreg attached to them and that's ok.

Here is what I would suggest:
- Change the comment to say that only alive registers are guaranteed to have a regclass.
- Change X86FastPreTileConfig to not process dead registers (see inline comment for actual fix)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157458