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

[Pengfei Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:16
 ///
-///  is transformed to
+/// The shape register is caller saved according to ABI. We need to insert
+/// ldtilecfg again after the call instruction if callee clobbers any AMX
----------------
LuoYuanke wrote:
> Do you mean all tile registers are caller saved?
No, tile registers are visible to RA, we don't need to restore them ourselves.
We are mainly doing the restore for the shape register in this pass.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:193
+            MachineInstr *MI2 = MO2.getParent();
+            if (MI2->isMoveImmediate() || MI2->isPHI())
+              continue;
----------------
LuoYuanke wrote:
> Do we need to recursively handle phi instruction? In below example, record "def shape" instruction?
> 
> def shape
>     \
>     phi
>       \
>      phi
I don't think so. Each phi will be iterated once. So we just need to peek its latest level and ignore the phi in it.
```
s1 s2
 \ /
  p1  s3  ...
   \  /   /
    p2   p3
     \  /
      p4
```
For exampe, if we meet p4 firstly, we find all its defs are phis, we don't do anything. Then, we meet p2 in another iteration and record s3. Then p1 etc. The order doesn't metter, you can see all the real shape defs will be recorded without recursion.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:253
+        if (Pred == &MF.front())
+          CfgNeedInsert.insert(MIRef(Pred));
+        else
----------------
LuoYuanke wrote:
> So we would try to insert ldtilecfg at the entry if there is not shape dependency issue?
Yes. Here the insert points are candidates. We will sink them based on the shape defs. We will insert at the entry if shape dependency satisfies.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:305
+          for (auto *Succ : I.MBB->successors())
+            if (BBVisitedInfo[Succ].NeedTileCfgLiveIn)
+              WorkList.push_back(MIRef(Succ));
----------------
LuoYuanke wrote:
> Can we sink the ldtilecfg in below case? We need insert ldtilecfg to B4.
> 
> ```
>                 Entry
>               /         \
>             B1         B2
>            call        call
>               \           /
>                   B3
>                     |
>                B4 shape defs
>                     |
>                  AMX
> 
> ```
Yes. We will sink two candidates from B1 anf B2. (If there's no call in B2, the candidates are B1 and Entry) We avoid BB being multi visited. So the second sink will stop at B3. This first sink will cross B3 and reach B4.


================
Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:329
     }
-    if (!UsableRegs.none())
-      addFrameReference(BuildMI(*I->getParent(), ++I->getIterator(), DebugLoc(),
-                                TII->get(X86::LDTILECFG)),
-                        FI);
-  }
-}
+  };
 
----------------
LuoYuanke wrote:
> Is this semicolon necessary?
Good catch. It's residue from previous lambda expression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99010