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

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added a comment.

Perhaps we need more comments and more test cases (maybe in a sperate file) to cover those scenario.



================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:127
+        : DFSPredsUpdater(PTC, MBB) {}
+    void update(MachineBasicBlock *MBB, MachineBasicBlock *Succ) {
+      if (PTC->BBVisitedInfo[MBB].PostDominateAllShape &&
----------------
Better to add comments to say shape is defined in Succ, so the pred won't postdominate the shape.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:129
+      if (PTC->BBVisitedInfo[MBB].PostDominateAllShape &&
+          (!PTC->MLI->isLoopHeader(Succ) ||
+           PTC->MLI->getLoopFor(Succ)->getBottomBlock() != MBB)) {
----------------
Here need comments to explain why the except is needed. 


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:195
+
+    ShapeT Shape(&MI.getOperand(Row), &MI.getOperand(Col), MRI);
+    for (auto *ShapeMO : {Shape.getRow(), Shape.getCol()}) {
----------------
Better to split the function into 2 function. One is isAMXInstruction(), the other is collectShapeInfo().


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:281
+    MachineBasicBlock *MBB = I->getParent();
+    while (!BBVisitedInfo[MBB].PostDominateAllShape) {
+      MachineBasicBlock *Next = nullptr;
----------------
Add comment to explain why the loop is needed, and how to find a position to insert.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:283
+      MachineBasicBlock *Next = nullptr;
+      if (BBVisitedInfo[MBB].FirstAMX)
+        REPORT_CONFIG_FAIL;
----------------
Add comment to explain why it fails.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:285
+        REPORT_CONFIG_FAIL;
+      std::for_each(MBB->succ_begin(), MBB->succ_end(), [&](auto *Succ) {
+        if (BBVisitedInfo[Succ].NeedTileCfgLiveIn) {
----------------
If possible, better add a test case for it.



================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:297
+    if (BBVisitedInfo[MBB].FirstAMX &&
+        BBVisitedInfo[MBB].FirstAMX < BBVisitedInfo[MBB].LastShape)
+      REPORT_CONFIG_FAIL;
----------------
Is it possible that LastShape is null?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99010