[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:59
+             "Must be a FRem instruction.");
+      assert((NumElements != nullptr && ElementType != nullptr) &&
+             "Vectorization factor or element type missing.");
----------------
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.


================
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),
----------------
IR builder has getAllOnesMask please use it instead


================
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");
+  }
----------------
this assert can be moved up so you won't need a temporary variable


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


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


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


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:135-138
+  StringRef ScalarName =
+      (ElementType->isFloatTy())
+          ? TLI.getName(LibFunc_fmodf)
+          : ((ElementType->isDoubleTy()) ? TLI.getName(LibFunc_fmod) : "");
----------------
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.



================
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;
----------------
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?


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


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


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:268
+  // replaced with calls to the vector library.
+  for (auto *I : ReplacedCalls) {
+    I->eraseFromParent();
----------------
could you can remove the braces here?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sleef-calls-aarch64.ll:2
 ; Do NOT use -O3. It will lower exp2 to ldexp, and the test will fail.
-; RUN: opt -vector-library=sleefgnuabi -replace-with-veclib < %s | opt -vector-library=sleefgnuabi -passes=inject-tli-mappings,loop-unroll,loop-vectorize -S | FileCheck %s --check-prefixes=CHECK,NEON
-; RUN: opt -mattr=+sve -vector-library=sleefgnuabi -replace-with-veclib < %s | opt -vector-library=sleefgnuabi -passes=inject-tli-mappings,loop-unroll,loop-vectorize -S | FileCheck %s --check-prefixes=CHECK,SVE
+; RUN: opt -vector-library=sleefgnuabi -passes=inject-tli-mappings,loop-unroll,loop-vectorize -S < %s | FileCheck %s --check-prefixes=CHECK,NEON
+; RUN: opt -mattr=+sve -vector-library=sleefgnuabi -passes=inject-tli-mappings,loop-unroll,loop-vectorize -S < %s | FileCheck %s --check-prefixes=CHECK,SVE
----------------
jolanta.jensen wrote:
> mgabka wrote:
> > could you explain why did you decide to modify this test?
> As I understand [[ https://github.com/llvm/llvm-project/commit/6577cef9b03fb4d151bd997a720ceaf78447f6a2 | the commit message  ]] and the implementation itself, ReplaceWithVeclib pass is to be run on intrinsics operating on vectors not scalar values. This pass did make any changes to the IR and is only confusing.
yes, you are correct, am I aware that this test is quite bad from the beginning. I would like to rearrange the SLEEF tests to match what I did for armpl in https://reviews.llvm.org/home/menu/view/137/, i.e have separate tests for LV and for replace with veclib.



For now I would leave it as it is and remove the code you added,  AFAIK we should not see calls to @fmod or @fmodf in the situations we want to use _ZGVsMxvv_fmod or _ZGVsMxvv_fmodf, in those cases frem instructions with vector operands should be present in IR instead.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156439