[PATCH] D156439: [TLI][AArch64] Extend ReplaceWithVeclib to replace vector FREM instructions for scalable vectors

[mgabka via Phabricator via llvm-commits]

mgabka added inline comments.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:53
   if (!TLIFunc) {
-    TLIFunc = Function::Create(OldFunc->getFunctionType(),
-                               Function::ExternalLinkage, TLIName, *M);
-    TLIFunc->copyAttributesFrom(OldFunc);
-
+    if (!CI) {
+      // FRem handling.
----------------
I think most of us are more used to read positive conditions this is why I would suggest to write it as:
if(CI) {
handle intrinsic
}
else {
handle frem and here the assert to check that the instruction is a frem would make more sense
}

I would suggest to do a simila change in the if/else block you added below


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:62
+      if (Masked)
+        Tys.push_back(ToVectorTy(IRBuilder.getInt1Ty(), NumElements));
+      TLIFunc = Function::Create(FunctionType::get(RetTy, Tys, false),
----------------
you can just use here: Tys.push_back(VectorType::get(Type::getInt1Ty(M->getContext()), NumElements));



================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:73
+      TLIFunc->copyAttributesFrom(OldFunc);
+      assert(OldFuncTy == TLIFunc->getFunctionType() &&
+             "Expecting function types to be identical");
----------------
IMO  this assert is redundant,  it is essentially checking if the Function::Create works correctly, what is not needed in my opinion.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:83
     // similar to as it is done in InjectTLIMappings
     appendToCompilerUsed(*M, {TLIFunc});
 
----------------
I know that this is not part of your work, but I realised that this is not tested at all, could you create a an NFC patch with regenerated tests with "--check-globals"  same applied to the non scalable test and rebase your patch?


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:103
   }
-
   // Convert vector arguments to scalar type and check that
----------------
not needed change


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:136
+  std::string TLIName =
+      std::string(TLI.getVectorizedFunction(ScalarName, NumElements));
+  if (!TLIName.empty()) {
----------------
this is wrong, please read about StringRef https://discourse.llvm.org/t/std-string-vs-llvm-stringref/65873/2

you can just write it as :

StringRef TLIName = TLI.getVectorizedFunction(ScalarName, NumElements


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:137-141
+  if (!TLIName.empty()) {
+    LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Found unmasked TLI function `"
+                      << TLIName << "`.\n");
+    return replaceWithTLIFunction(I, TLIName);
+  }
----------------
I am not sure that this is even needed, in the TLI all mappings for SLEEF or ArmPL  for scalable vectors are masked, so I would assume that we either replace with masked call or we do not replace at all, @paulwalker-arm what is your opinion?


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:147
+                      << TLIName << "`.\n");
+    return replaceWithTLIFunction(I, TLIName, true);
+  }
----------------
I think LLVM prefers to use an in-line C-style comment in this case /*Masked*/ true


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:149
+  }
+  LLVM_DEBUG(dbgs() << DEBUG_TYPE
+                    << ": Did not find suitable vectorized version of `"
----------------
I think this is going to pollute dbg log too much as it will add a debug message for each unsupported intrinsic, please remove it.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:248
+    } else if (I.getOpcode() == Instruction::FRem) {
+      // If there is a suitable TLI mapping for FRem instruction,
+      // replace the instruction.
----------------
nit: better to use consistent spelling so either please use FRem of frem everywhere


================
Comment at: llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll:370
+
+define <vscale x 2 x double> @frem_vscale_f64(<vscale x 2 x double> %in1, <vscale x 2 x double> %in2) {
+; CHECK-LABEL: @frem_vscale_f64(
----------------
please use same naming scheme as other functions in this file so this should be : llvm_frem_vscale_f64, please apply to the other tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156439