[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:55
+  MachineBasicBlock *MBB = nullptr;
+  size_t Pos = 0; /* A virtual position for instr will be inserted after MI */
+  MIRef() = default;
----------------
use C++ comments // between line 54,55


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:67
+      : MI(MI), MBB(MBB), Pos(Pos) {}
+  MachineInstr *operator->() { return MI; }
+  operator bool() const { return MBB != nullptr; }
----------------
use getMI() or directly p->MI is more readable.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:108
+  /// Check if MI is AMX instruction.
+  bool isAMXInstruction(MachineInstr &MI) {
+    switch (MI.getOpcode()) {
----------------
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.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:188
+        for (unsigned Index = 0; Index < MI->getNumOperands() / 2; ++Index) {
+          Register DefReg = MI->getOperand(Index * 2 + 1).getReg();
+          for (MachineOperand &MO2 : MRI->def_operands(DefReg)) {
----------------
here may be a constant imm in PHI's incomings.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:193
+            MachineInstr *MI2 = MO2.getParent();
+            if (MI2->isMoveImmediate() || MI2->isPHI())
+              continue;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99010