[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:59
+             "Must be a FRem instruction.");
+      assert((NumElements != nullptr && ElementType != nullptr) &&
+             "Vectorization factor or element type missing.");
----------------
mgabka wrote:
> the NumElements and ElementType you can obtain from the Frem instruction return tyoe, so you do not need to pass it as arguments to this function and you can remove this assert as well.
I would need to do a cast similar way as I do in replaceFremWithCallToVeclib, i.e. something like (dyn_cast<ScalableVectorType>(I.getType()))->getElementCount() and I would need a variable for it in the whole function scope even if only frem is using it. I think it's less fuss to send it as argument. But if you think it's better to obtain it from Frem instruction anyway, I'll change.  
I realized I do not use ElementType and I removed it.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:65
+      if (Masked)
+        Tys.push_back(ToVectorTy(IRBuilder.getInt1Ty(), *NumElements));
+      TLIFunc = Function::Create(FunctionType::get(RetTy, Tys, false),
----------------
mgabka wrote:
> IR builder has getAllOnesMask please use it instead
It does similar except I'll get a Value not Type since it does more than I need and I would need to convert back. 

```
  Value *getAllOnesMask(ElementCount NumElts) {
    VectorType *VTy = VectorType::get(Type::getInt1Ty(Context), NumElts);
    return Constant::getAllOnesValue(VTy);
  }
```
I kept line 65 as it is but I changed code on lines 98-100 to use getAllOnesMask.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:111-112
+    Replacement = IRBuilder.CreateCall(TLIFunc, Args, OpBundles);
+    assert(OldFuncTy == TLIFunc->getFunctionType() &&
+           "Expecting function types to be identical");
+  }
----------------
mgabka wrote:
> this assert can be moved up so you won't need a temporary variable
Fixed.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:116
   if (isa<FPMathOperator>(Replacement)) {
     // Preserve fast math flags for FP math.
+    Replacement->copyFastMathFlags(&I);
----------------
mgabka wrote:
> could you move this comment abve the if stmt and remove the braces?
Fixed.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:122
   ++NumCallsReplaced;
+
   return true;
----------------
mgabka wrote:
> extra new line, it is not needed
Fixed.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:130
+  if (!VectorArgTy) {
+    // We have TLI mappings for FRem on scalable vectors only.
     return false;
----------------
mgabka wrote:
> could you move this comment abve the if stmt and remove the braces?
Fixed.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:135-138
+  StringRef ScalarName =
+      (ElementType->isFloatTy())
+          ? TLI.getName(LibFunc_fmodf)
+          : ((ElementType->isDoubleTy()) ? TLI.getName(LibFunc_fmod) : "");
----------------
mgabka wrote:
> IMO this would be more readable if written as if statements, moreover if the type is not double of float we should rerutn false from this function, am I right?
> 
> StringRef ScalarName;
> if (ElementType->isFloatTy())
>   ScalarName =TLI.getName(LibFunc_fmodf);
> elseif(ElementType->isDoubleTy())
>   ScalarName =TLI.getName(LibFunc_fmod)
> else
> return false;
> 
> thanks to to the statement below is not needed and code is more clear and compact.
> 
Fixed. In the unusual case the ScalarName is empty we will just do 2 unnecessary lookups.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:142-143
+  if (!TLI.isFunctionVectorizable(ScalarName)) {
+    // The TargetLibraryInfo does not contain a vectorized version of
+    // the scalar function.
+    return false;
----------------
mgabka wrote:
> could you move the comments above the if and remove the braces?
> 
> I think this is not needed entirely to be fair, can we rely on the check performed by TLI.getVectorizedFunction? and add debug message just below the final return false?
Yes, we can. It calls into:
```
  bool isFunctionVectorizable(StringRef F, const ElementCount &VF) const {
    return !(getVectorizedFunction(F, VF, false).empty() &&
             getVectorizedFunction(F, VF, true).empty());
  }
```
Removed the isFunctionVectorizable() check and added a debug message if a vectorized function wasn't found.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:170
+  if (I.getOpcode() == Instruction::FRem) {
+    // Replacement can be performed for FRem instruction.
+    return replaceFremWithCallToVeclib(TLI, I);
----------------
mgabka wrote:
> please move the comment above the if stmt and remove braces
Fixed.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:174
+  CallInst *CI = dyn_cast<CallInst>(&I);
+  if (!CI)
+    return false;
----------------
mgabka wrote:
> these 2 if stmts can be combined together, C/C++ guarantees that they are executed from left to right
Fixed.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:268
+  // replaced with calls to the vector library.
+  for (auto *I : ReplacedCalls) {
+    I->eraseFromParent();
----------------
mgabka wrote:
> could you can remove the braces here?
Fixed. I kept the a blank line before return to make it more visible what is returned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156439