[PATCH] D101067: [X86][AMX] Try to hoist AMX shapes' def

[Pengfei Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:199
     MIRef MIR(MI, MBB);
-    if (BBVisitedInfo[MBB].LastShape < MIR)
-      BBVisitedInfo[MBB].LastShape = MIR;
-    ShapeBBs.insert(MBB);
+    auto I = llvm::lower_bound(ShapeBBs[MBB], MIR);
+    if (*I != MIR)
----------------
xiangzhangllvm wrote:
> ShapeBBs[MBB] may both contains the Row_def and Col_def, and they may be in different BBs, The Pos of them in their BB my be equal, So use lower_bound here seems not good. Because it may duplicated insert X_def at line 201.
`ShapeBBs[MBB]` contains Shapes only defined in the same `MBB`. We don't distinguish Row and Col here.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:264
+    if (BBVisitedInfo[&MBB].FirstAMX || BBVisitedInfo[&MBB].HasAMXRegLiveIn)
+      for (auto *Succ : MBB.successors())
+        if (!isLoopBackEdge(Succ, &MBB))
----------------
xiangzhangllvm wrote:
> Looks strange here: All BB followed a AMX BB will be mark HasAMXRegLiveIn, Even BBs after the last BB of using AMX?
It's true from perspective of the live range of TMM registers. Though might be not precise. (The live range actually ends after function call). We simply use such conception here to check if there's any shape def BBs have AMX registers live in, i.e. the shapes are interact with AMX instructions.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:292
+  for (auto &I : ShapeBBs) {
+    // TODO: We can hoist shapes across BBs here.
+    if (BBVisitedInfo[I.first].HasAMXRegLiveIn)
----------------
xiangzhangllvm wrote:
> I think "across BBs" is most common case:
> 
> AMX BB0  --> shape def  --> AMX BB1
We can improve it in follow up patches :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101067