[PATCH] D100026: [X86] Support AMX fast register allocation

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added a comment.

I'll try build it in release, make sure it no warnings, thanks!



================
Comment at: llvm/include/llvm/CodeGen/CodeGenPassBuilder.h:1042
   addRegAllocPass(addPass, false);
+  addPostFastRegAllocRewrite(addPass);
   return Error::success();
----------------
pengfei wrote:
> Is this function actually used?
I think I mix with the same named function "bool addPostFastRegAllocRewrite( )", thank you!


================
Comment at: llvm/lib/Target/X86/X86LowerAMXType.cpp:68
+// as a new Row for other new created AMX intrinsics.
+static std::map<Value *, Value *> Col2Row;
+
----------------
pengfei wrote:
> Better move it to class `X86LowerAMXType`
I thought it before, here I just won't to pass it into static function (getRowFromCol).


================
Comment at: llvm/lib/Target/X86/X86LowerAMXType.cpp:370-372
+static Value *getAllocaPos(BasicBlock *BB) {
+  Module *M = BB->getModule();
+  Function *F = BB->getParent();
----------------
pengfei wrote:
> You can make it be member of X86VolatileTileData. Then you don't need to calculate `F` here.
this is in static function, not function of X86VolatileTileData.


================
Comment at: llvm/lib/Target/X86/X86PreAMXConfig.cpp:205-207
+  preWriteTileCfg(I8Ptr, Cfg, Shapes);
+
+  return Cfg;
----------------
pengfei wrote:
> `return preWriteTileCfg(I8Ptr, Cfg, Shapes);` ?
Let's return Init0, cover the assert(Init0 && ...), thanks!


================
Comment at: llvm/lib/Target/X86/X86PreAMXConfig.cpp:324
+        continue;
+      assert(onlyTileDef(II) && "Not volatile model for AMX at O0!");
+
----------------
pengfei wrote:
> Does it cause warning in release build?
I think yes, good catch, thanks!


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

https://reviews.llvm.org/D100026