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

[Jolanta Jensen via Phabricator via llvm-commits]

jolanta.jensen 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.
----------------
mgabka wrote:
> 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
I agree. It was ordered this way because frem handling was before intrinsics handling in replaceWithCallToVeclib. But since branching was reverted to reside in runImpl where it was in thefirst version of the patch, it does not make sense to have intrinsics handling before frem handling here. Hopefully the code will be more clear this way even if I dont like those if...else... anyway and I would prefer separate functions  for intrinsics and frem.


================
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),
----------------
mgabka wrote:
> you can just use here: Tys.push_back(VectorType::get(Type::getInt1Ty(M->getContext()), NumElements));
> 
Fixed. This way I could move declaration of IRBuilder more locally too.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:73
+      TLIFunc->copyAttributesFrom(OldFunc);
+      assert(OldFuncTy == TLIFunc->getFunctionType() &&
+             "Expecting function types to be identical");
----------------
mgabka wrote:
> IMO  this assert is redundant,  it is essentially checking if the Function::Create works correctly, what is not needed in my opinion.
This assert is part of the original implementation, removing it would be introducing unnecessary nfc (even if it has been moved to avoid creation of an extra variable -- so the move is a nfc anyway). I would like to keep it since it is a part of the original implementation.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:83
     // similar to as it is done in InjectTLIMappings
     appendToCompilerUsed(*M, {TLIFunc});
 
----------------
mgabka wrote:
> 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?
Fixed.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:136
+  std::string TLIName =
+      std::string(TLI.getVectorizedFunction(ScalarName, NumElements));
+  if (!TLIName.empty()) {
----------------
mgabka wrote:
> 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
Yeah, copy-pasted from the original code in replaceWithCallToVeclib. Corrected -- but only here, in my own code.


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


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:149
+  }
+  LLVM_DEBUG(dbgs() << DEBUG_TYPE
+                    << ": Did not find suitable vectorized version of `"
----------------
mgabka wrote:
> 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.
Why should it pollute? We won't get it more times than

```
 LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Looking up TLI mapping for `"
                    << ScalarName << "` and vector width " << NumElements
                    << ".\n");
```
It matches the above print in case we find nothing. I think it's user friendly and I would like to keep 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.
----------------
mgabka wrote:
> nit: better to use consistent spelling so either please use FRem of frem everywhere
Fixed.


================
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(
----------------
mgabka wrote:
> 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
The other test names start with @llvm since they call functions which names start with @llvm. My test names start with @frem since I call frem. In my opinion it is consistent with the naming scheme in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156439