[PATCH] D95136: [X86] Fix tile config register spill issue.

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:230
+struct ReloadTileConfig {
+  enum SuccStatus { NoAMXAtAll = 0, HasAfterCallAMX, HasBeforeCallAMX };
+
----------------
Better to have a diagram comments to illustrate the state.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:235
+    bool NeedUpdatePred = true;
+    bool HasCallBeforeAMX = false;
+    SuccStatus MaxSucc = NoAMXAtAll;
----------------
xiangzhangllvm wrote:
> Can use a better name for HasCallBeforeAMX? it is very easy to mix with HasAfterCallAMX when I read here
Agree. It is easy to mix with HasBeforeCallAMX.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:271
+    WorkList.push_back(MBB);
+    while (WorkList.size()) {
+      MBB = WorkList.pop_back_val();
----------------
!WorkList.empty()?


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:283
+    // But iterating all BBs isn't too expensive here.
+    for (auto I : BBVisitedInfo) {
+      if (I.second.NeedUpdatePred)
----------------
xiangzhangllvm wrote:
> The logic here is complex, better to add clear comment to show how we update the Status for BBs.
WorkList should be clear before this loop?


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:286
+        WorkList.push_back(I.first);
+      while (WorkList.size()) {
+        MBB = WorkList.pop_back_val();
----------------
(!WorkList.empty()) seems more readable.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:297
+        for (auto I = MBB->pred_begin(), E = MBB->pred_end(); I != E; ++I) {
+          if (!BBVisitedInfo.count(*I))
+            continue;
----------------
Not quite understand this. Isn't all BBs added at line 275?


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:302
+            // If BB has AMX, its predecessors don't need to be updated again.
+            if (!BBVisitedInfo[*I].HasAMX)
+              BBVisitedInfo[*I].NeedUpdatePred = true;
----------------
Don't understand this. Can you add a diagram to illustrate it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95136