[PATCH] D99010: [X86][AMX] Hoist ldtilecfg

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:108
+  /// Check if MI is AMX instruction.
+  bool isAMXInstruction(MachineInstr &MI) {
+    switch (MI.getOpcode()) {
----------------
pengfei wrote:
> xiangzhangllvm wrote:
> > I suggest use tile operand to judge if a MI is AMX Instruction or not. and spilt the collection of shape to another special function.
> But we still need to judge if it is PHI. I don't know if there're other exceptions.
> We have split it to collectShapeInfo.
Yes, PHI is exception, the exceptions is very few, more better than list all the AMX opcodes.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:193
+            MachineInstr *MI2 = MO2.getParent();
+            if (MI2->isMoveImmediate() || MI2->isPHI())
+              continue;
----------------
pengfei wrote:
> xiangzhangllvm wrote:
> > pengfei wrote:
> > > LuoYuanke wrote:
> > > > Do we need to recursively handle phi instruction? In below example, record "def shape" instruction?
> > > > 
> > > > def shape
> > > >     \
> > > >     phi
> > > >       \
> > > >      phi
> > > I don't think so. Each phi will be iterated once. So we just need to peek its latest level and ignore the phi in it.
> > > ```
> > > s1 s2
> > >  \ /
> > >   p1  s3  ...
> > >    \  /   /
> > >     p2   p3
> > >      \  /
> > >       p4
> > > ```
> > > For exampe, if we meet p4 firstly, we find all its defs are phis, we don't do anything. Then, we meet p2 in another iteration and record s3. Then p1 etc. The order doesn't metter, you can see all the real shape defs will be recorded without recursion.
> > 1) MI2->isMoveImmediate() may need to be handled, because the imm may always keep in a register after PHI.
> > In other wards, if AMX MI use the imm shape, they must use the register after PHI. 
> > 
> > 2) the recursive PHI case is very rare, we can let it easy, not handle it, there is not correction problem.
> 1. The PostRA pass can handle this case.
> 2. We have covered this case here.
I don't think  "Each phi will be iterated once."

MRI->def_operands(ShapeReg) only contains the shape reg used in AMX MI


```
s1 s2
 \ /
  p1  s3  ...
   \  /   /
    p2   p3
     \  /
      p4
 then AMX MI use p4 as this shape,
```

 only p4 in def_operands(ShapeReg).


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:193
+            MachineInstr *MI2 = MO2.getParent();
+            if (MI2->isMoveImmediate() || MI2->isPHI())
+              continue;
----------------
xiangzhangllvm wrote:
> pengfei wrote:
> > xiangzhangllvm wrote:
> > > pengfei wrote:
> > > > LuoYuanke wrote:
> > > > > Do we need to recursively handle phi instruction? In below example, record "def shape" instruction?
> > > > > 
> > > > > def shape
> > > > >     \
> > > > >     phi
> > > > >       \
> > > > >      phi
> > > > I don't think so. Each phi will be iterated once. So we just need to peek its latest level and ignore the phi in it.
> > > > ```
> > > > s1 s2
> > > >  \ /
> > > >   p1  s3  ...
> > > >    \  /   /
> > > >     p2   p3
> > > >      \  /
> > > >       p4
> > > > ```
> > > > For exampe, if we meet p4 firstly, we find all its defs are phis, we don't do anything. Then, we meet p2 in another iteration and record s3. Then p1 etc. The order doesn't metter, you can see all the real shape defs will be recorded without recursion.
> > > 1) MI2->isMoveImmediate() may need to be handled, because the imm may always keep in a register after PHI.
> > > In other wards, if AMX MI use the imm shape, they must use the register after PHI. 
> > > 
> > > 2) the recursive PHI case is very rare, we can let it easy, not handle it, there is not correction problem.
> > 1. The PostRA pass can handle this case.
> > 2. We have covered this case here.
> I don't think  "Each phi will be iterated once."
> 
> MRI->def_operands(ShapeReg) only contains the shape reg used in AMX MI
> 
> 
> ```
> s1 s2
>  \ /
>   p1  s3  ...
>    \  /   /
>     p2   p3
>      \  /
>       p4
>  then AMX MI use p4 as this shape,
> ```
> 
>  only p4 in def_operands(ShapeReg).
But here missed the def shape BB, it is not meet the following logic of use "shapeBBNumber == ShapeReachedCount" to judge the BB is all shape def reachable.


```
   reg=imm        ...
   /     \         / 
amx          phi
              |
             BB   
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99010