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

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:13
 /// The pldtilecfg is to config tile registers. It should dominator all AMX
 /// instructions. The pldtilecfg produce a virtual cfg register and the cfg
 /// register is used by all AMX instructions.
----------------
Since we change the algorithm, we need to update the pass description.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:73
+                                    MI->getIterator())) {}
+    MIRef(MachineInstr *MI, unsigned Pos) : MI(MI), Pos(Pos) {}
+    MIRef(MachineBasicBlock::iterator MII)
----------------
In line 68 Pos is size_t, but the input is unsigned. Need to align the type?


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:103
+    MIRef LastShape;
+    bool DominateAllShape = true;
+    bool HasAMXBeforeCall = false;
----------------
PostDominateAllShapes.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:104
+    bool DominateAllShape = true;
+    bool HasAMXBeforeCall = false;
+  };
----------------
This looks to represent the tile register live through MBB in the code. Basically we need to reconfig after last call if the config register live out the MBB.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:131
+      } else if (!PTC->BBVisitedInfo[MBB].HasAMXBeforeCall) {
+        PTC->BBVisitedInfo[MBB].HasAMXBeforeCall = true;
+        WorkList.push_back(MBB);
----------------
Does HasAMXBeforeCall mean amx tile config register live through the MBB?


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:140
+        : DFSPredsUpdater(PTC, MBB) {}
+    void update(MachineBasicBlock *MBB, MachineBasicBlock *Parent) {
+      if (PTC->BBVisitedInfo[MBB].DominateAllShape &&
----------------
Maybe rename it as "update(MachineBasicBlock *Pred, MachineBasicBlock *MBB)"


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:174
+            BBVisitedInfo[MBB].FirstAMX = MIRef(&MI, Pos);
+          if (BBVisitedInfo[MBB].LastCall)
+            CfgNeedInsert.insert(BBVisitedInfo[MBB].LastCall);
----------------
Better to comment on each condition.


```
call
here we need to reload tile config.
amx instr

```



================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:177
+          else if (BBVisitedInfo[MBB].FirstAMX.MI == &MI)
+            BBVisitedInfo[MBB].HasAMXBeforeCall = true;
+        } else if (MI.isCall() && isDestructiveCall(MI)) {
----------------
Does HasAMXBeforeCall mean amx config register live in MBB?


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:205
 
-  MachineInstr *getTileConfigPoint();
+    if (!MLI)
+      MLI = &getAnalysis<MachineLoopInfo>();
----------------
We can initialize at the beginning. Maybe at runOnMachineFunction().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99010