[PATCH] D157458: [X86] Run X86FastPreTileConfigPass only with FastISel.

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

> I've added a part about alive registers.

I didn't see that part. I've suggested one as a result (https://reviews.llvm.org/D157458?id=548421#inline-1527304 and https://reviews.llvm.org/D157458?id=548421#inline-1527304).



================
Comment at: llvm/include/llvm/CodeGen/MachineRegisterInfo.h:675
   /// 2. The machine function has not completely been through the
-  ///    instruction selection process.
+  ///    instruction selection process or selected using GlobalISel.
   /// None of this condition is possible without GlobalISel for now.
----------------
qcolombet wrote:
> pengfei wrote:
> > pengfei wrote:
> > > Is GlobalISel also a instruction selection process?
> > I got your point. I think it's better to list as 3 conditions:
> > ```
> > 2. MIR selected using GlobalISel.
> > 3. The machine function has ..
> > ```
> GlobalISel is an instruction selection process.
> The current sentences make sense the way they are, I think.
Instead of modifying #2, I would say:
```
2. [...], or
3. The virtual register is dead and didn't get through the selection process.
```


================
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.
----------------
e-kud wrote:
> 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.
Maybe we could simplify the sentence and only say, it is safe to use `getRegClass` when we are after isel and reg is alive (`!MRI->reg_empty(VirtReg)`).
I.e., as long as we don't iterate over dead registers, this is safe no matter what.


================
Comment at: llvm/lib/Target/X86/X86FastPreTileConfig.cpp:672
+    const TargetRegisterClass *RC = MRI->getRegClassOrNull(VirtReg);
+    if (RC && RC->getID() == X86::TILERegClassID) {
       HasVirtTileReg = true;
----------------
e-kud wrote:
> qcolombet wrote:
> > I think the proper fix here would be:
> > ```
> >  if (!MRI->reg_empty(VirtReg) && // <-- only process alive registers
> >      MRI->getRegClass(VirtReg)->getID() == X86::TILERegClassID) {
> > ```
> @qcolombet thank you! This fix is really better than just skipping registers. However since X86FastPreTileConfig is intended for FastISel only. I've decided to continue this way.
Should you assert that RC is not `nullptr` then?

And add a message that this pass is intended to run after FastISel.

E.g., `assert(RC && "This shouldn't happen when registers are created through FastISel");`


================
Comment at: llvm/test/CodeGen/X86/AMX/amx-fastpreconfig-gisel.mir:6
+# registers without class.
+# Note that %3 doesn't have a class.
+--- |
----------------
I guess I'll phrase the test case differently:
```
Make sure `fastpretileconfig` doesn't choke on virtual registers that didn't get through the instruction selection process.
This may happen when a virtual register is created during the isel process and that code gets optimized away before it gets through the final instruction selection stage.

In this particular test, this can be seen with reg id 3 having no register class (only a register bank).
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157458