[PATCH] D125075: [X86][AMX] Multiple configure for AMX register.

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/Target/X86/X86FastPreTileConfig.cpp:550
+    if (HasTileOperand(MRI, MI))
+      HasUnconfigTile = true;
+    // According to AMX ABI, all the tile registers including config register
----------------
xiangzhangllvm wrote:
> I am afraid , even without call, 1 ldtilecfg is not enough for 1 MBB.
> For example
> In 1 MBB, it contain 4 shapes, but the first 3 shape used up the "max reg num of ldtilecfg (8)" virtual tile regs in follow code. So It need another ldtilecfg.
That would run out of register. Currently we have valotiled tile in "lower amx type" pass. We can improve it later and disable volatile tile in that pass.


================
Comment at: llvm/lib/Target/X86/X86FastPreTileConfig.cpp:564
+    if (!isTileDef(MRI, MI))
+      continue;
+    // If MI dominate the last shape def instruction, we need insert
----------------
xiangzhangllvm wrote:
> Not much sure here must no "COPY" for tile.  (T1 = Copy T0)
> Better to add assert here to exclude unexpected Tile def (MI is not intrinsic).
Thre are only phi or copy beside AMX intrinsic. We handle phi separately. "COPY" may be generated after phi elimination. OK, let me add assert here.


================
Comment at: llvm/lib/Target/X86/X86FastPreTileConfig.cpp:574
+    if (LastShapeMI && dominates(MBB, MI, LastShapeMI))
+      Config(*(++LastShapeMI->getIterator()));
+    MachineOperand *RowMO = &MI.getOperand(1);
----------------
xiangzhangllvm wrote:
> Sorry, In my understand, the comment is not match the code. 
> it should be
> 
> ```
>     //   tilezero
>     //   def row
>     //   def col  <- LastShapeMI 
>    //   ldtilecfg <- insert
>     //   tilezero(row, col)
> ```
Let me update the comments.


================
Comment at: llvm/lib/Target/X86/X86FastPreTileConfig.cpp:604
+        // reload befor UseMI
+        reload(UseMI.getIterator(), TileReg, RowMO, ColMO);
+      } else {
----------------
xiangzhangllvm wrote:
> We should escape duplicated reload. for example:
> 
> Not re-gen reload for line 2.
> ```
> 1  T0 = TileLoad
> 2   TileUse T0
> ```
> 
How about optimize reload in another patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125075