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

[Pengfei Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:252
+            IsCallBeforeAMX = true;
+          LastCallMayHasAMX = &*MII;
+        }
----------------
LuoYuanke wrote:
> What about multi call and multi AMX in one BB? For below example, should we insert ldtilecfg after call1?
> 
> call1
> AMX1
> call2
> AMX2
Yes, we insert for call1 and call2 in line 247.
We only add the BB to `BBUnsolved` if there's no AMX after the last call.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:235
+    bool NeedUpdatePred = true;
+    bool HasCallBeforeAMX = false;
+    SuccStatus MaxSucc = NoAMXAtAll;
----------------
LuoYuanke wrote:
> pengfei wrote:
> > LuoYuanke wrote:
> > > 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.
> > Should `IsCallBeforeAMX` better?
> Maybe HasCallBeforeAMXInBB.
`HasCallBeforeAMXInBB` is still easy mix up with `HasAfterCallAMX`, besides, we need to mark it true even there's no AMX in the BB.


================
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;
----------------
LuoYuanke wrote:
> Not quite understand this. Isn't all BBs added at line 275?
No, we only added successors since ldtilecfg BB. BB0, BB1 and BB2 in below graph won't be added.
```
    BB0   ldtilecfg
     |      / \
    BB1   BB4 BB5
    / \   /   /
  BB2  BB3  BB6
     \  |  /
       BB7
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95136