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

Thu Aug 10 18:22:59 PDT 2023

pengfei added inline comments.

================
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
+
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157458