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

[jolanta.jensen]

Summary

This patch teaches ReplaceWithVeclib pass how to replace
vector FREM instructions for scalable vectors.

Depends on D157258

[mgabka]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

129–140

from what I can see this is the only different part of code then in "replaceWithTLIFunction" , hence most of the code below is a code duplication. I think a smart refactoring of replaceWithTLIFunction would be enough to make it work for CI and Instructions.

The other thing is that the code you wrote duplicates a lot, you can use a suitable container and just add extra element there when you are creating a type for masked function, in that way you do not need to create the type twice and add input types twice.

167

I guess that this is handled by "replaceAllUsesWith" but worth to check if with a test.

248

this is not needed, the mechanism here should work for both fixed and scalable types if mappings exist.
if we want to reject the transformation for scalable vector types I think we should reject it earlier, i.e where we detect frem.

please LLVM coding guideline on using braces with simple if statememts https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements, it applies to other places in this patch.

248–249

Hi Jolanta,
in my opinion it would be better to have a single main entry point here, and then branch from inside replaceWithCallToVeclib, thanks to it you can void some of the code duplication, like for example the debug messages.

251

when moving this outside this function, this check can be combined with the one above since ScalableVectorType class exists.

257–260

the function name "replaceInstructionWithCallToVeclib" does not suggest that this is specific to fmod/frem, at the moment we do not have intention to extend it beyond frem, so in my opinion it is worth to replace the function name, to avoid confusion.

261

please use early exit instead, it is much better idea to use early exits from function instead long nested if block, it improves code readability.
please apply it where possible to other checks you are doing

269–270

why looking for it in cases where you are not going to use it? it isn't efficient. please change it.

271–273

this message is printed actually after looking for the mappings, so probably should be moved up.

llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-armpl.ll

388

having both arguments of frem as inputs to this function would simplify the test, same apply to other tests.

The other thing is that other tests in this file are using fast math flags, so please use it for frem as well.

llvm/test/Transforms/LoopVectorize/AArch64/sleef-calls-aarch64.ll

2–3

could you explain why did you decide to modify this test?

[jolanta.jensen]

Addressed review comments.

[jolanta.jensen]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

129–140

Lines 129-134 also differ. Line 77 has an assert that is not vaIid here. I created small functions for lines 54-66 and 116-127 that are shared. And I fixed the code duplication above.

167

Indeed, it is handled by "replaceAllUsesWith".

void Value::replaceAllUsesWith(Value *New) {
  doRAUW(New, ReplaceMetadataUses::Yes);
}

void Value::doRAUW(Value *New, ReplaceMetadataUses ReplaceMetaUses) {
...
  if (ReplaceMetaUses == ReplaceMetadataUses::Yes && isUsedByMetadata())
    ValueAsMetadata::handleRAUW(this, New);
...

Do we need a test to confirm or is this explanation good enough?

248

There is no mappings for frem with fixed vectors for SLEEF library so we need to check it's a scalable type.

I'll check the code for breaches of the standard and I'll correct.

248–249

Fixed.

251

I changed to ScalableVectorType.

257–260

Fixed.

261

Fixed.

269–270

Fixed.

271–273

Fixed.

llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-armpl.ll

388

Fixed.

llvm/test/Transforms/LoopVectorize/AArch64/sleef-calls-aarch64.ll

2–3

As I understand 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.

[mgabka]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

129–140

I am not against having "replaceFremWithCallToVeclib" I think that it could stay.
However I would like you to try to not create " replaceFremWithCallToVeclib".
In my opinion the code will be more readable if we have only replaceWithTLIFunction, which takes an Instruction, and do not create functions like "replaceWithNewCallInst" or "addFunctionToCompilerUsed".
If it makes code more clean it might be worth to move some of the frem related things like creating function type to a helper function.

[jolanta.jensen]

Addressing review comment about refactoring.

[jolanta.jensen]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

129–140

Fixed. All frem handling is in replaceWithTLIFunction. I think it's better to have it all gathered there than break out into separate function(s) as it's not that much code.

[mgabka]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

57

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.

63

IR builder has getAllOnesMask please use it instead

90–91

could you move this comment abve the if stmt and remove the braces?

108–109

this assert can be moved up so you won't need a temporary variable

115

extra new line, it is not needed

122

could you move this comment abve the if stmt and remove the braces?

145–148

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.

152–153

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?

163

please move the comment above the if stmt and remove braces

167

these 2 if stmts can be combined together, C/C++ guarantees that they are executed from left to right

255

could you can remove the braces here?

llvm/test/Transforms/LoopVectorize/AArch64/sleef-calls-aarch64.ll

2–3

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.

[jolanta.jensen]

Addressing review comments.

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

90–91

Fixed.

122

Fixed.

145–148

Fixed. In the unusual case the ScalarName is empty we will just do 2 unnecessary lookups.

152–153

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.

163

Fixed.

167

Fixed.

[jolanta.jensen]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

57

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.

63

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.

108–109

Fixed.

115

Fixed.

255

Fixed. I kept the a blank line before return to make it more visible what is returned.

[jolanta.jensen]

Addressing review comments.

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

57

Fixed.

[jolanta.jensen]

Addressing yet another review comments.

[mgabka]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

53–75

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

62

you can just use here: Tys.push_back(VectorType::get(Type::getInt1Ty(M->getContext()), NumElements));

73

IMO this assert is redundant, it is essentially checking if the Function::Create works correctly, what is not needed in my opinion.

83

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?

103

not needed change

137

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

138–142

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?

148

I think LLVM prefers to use an in-line C-style comment in this case /*Masked*/ true

150

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.

252

nit: better to use consistent spelling so either please use FRem of frem everywhere

llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll

370

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

[paulwalker-arm]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

138–142

Checking for both is fine and akin to what we do in LoopVectorize. There's no requirement for scalable vector math routines to require a mask, it's just masked versions are easier to work with when tail-folding.

[jolanta.jensen]

Addressing review comments

[jolanta.jensen]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

53–75

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.

62

Fixed. This way I could move declaration of IRBuilder more locally too.

73

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.

83

Fixed.

137

Yeah, copy-pasted from the original code in replaceWithCallToVeclib. Corrected -- but only here, in my own code.

148

Fixed.

150

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.

252

Fixed.

llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll

370

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.

[jolanta.jensen]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

52

This is a bug. It will loose its value when it comes to line 104. It needs to be instantiated at once, i.e.
ElementCount NumElements = (dyn_cast<ScalableVectorType>(I.getType()))->getElementCount();

[jolanta.jensen]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

52

Or actually, the issue here is we set the ElementCount from an if statement, so if the TLIFunc is present, it will not be set.

[mgabka]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

51

this is wrong, i.e in cases where the function is already declared in the module you are not initialising it and in the line 112 you are going to print empty string.

The code below is adding function declaration to the module, and you are setting the OldName only in such situation similar situation applied to the NumElements variable.
So this is an incorrect code as those variables should be set to correct value in both situations.

I also believe the code below could be written in a bit shorter, please check if you can do anything with it as inspiration have a look at the code I suggested to use in the other place.

91–110

this can be written in much more compacted way for example:

SmallVector<Value *> Args(I.operand_values());
SmallVector<OperandBundleDef, 1> OpBundles;
// Preserve the operand bundles if it is an intrinsic call.
if (CI)
  CI->getOperandBundlesAsDefs(OpBundles);
// for masked calls to frem add a mask operand =
else if (Masked)
       Args.push_back(IRBuilder.getAllOnesMask(NumElements));

CallInst *Replacement = IRBuilder.CreateCall(TLIFunc, Args, OpBundles);
if (isa<FPMathOperator>(Replacement)) {
    // Preserve fast math flags for FP math.
    Replacement->copyFastMathFlags(&I);
  }

103

not needed change please revert this change.

150

I was too fast, this function only works for frem i nstruction, not for all llvm intrinsics.
In my view there is no need to add messages when optimization won't happen, you added messages when it happens, so lack of it means that it is not happening.

llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll

370

In my opinion it makes things only more difficult as it breaks the alphabetical order, and when looking for output for frem we need to too at. the end of the file instead place where all math operations started which name starts with "f" is.

[mgabka]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

91–110

I was wrong here, when the Instruction is a CI has the call as the first operand so it needs to be a bit different, I also think that it would be good to handle always mask (assuming it is always the last operand)

SmallVector<Value *> Args;
SmallVector<OperandBundleDef, 1> OpBundles;
// Preserve the operand bundles and copy arguments if it is an intrinsic call.
if (CI) {
  Args.assign(CI->arg_begin(), CI->arg_end());
  CI->getOperandBundlesAsDefs(OpBundles);
 }
else
  Args.assign(I.op_begin(), I.op_end());
// if mask is requested we need to add it
if (Masked)
  Args.push_back(IRBuilder.getAllOnesMask(NumElements));

CallInst *Replacement = IRBuilder.CreateCall(TLIFunc, Args, OpBundles);

[jolanta.jensen]

Bug fixes and review comments.

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

51

Yup. this is a bug as well. Corrected.

The code below (everything that is in if (CI) block) is part of the original implementation and I would prefer not to touch it.

91–110

This is part of the original implementation and I would prefer not to touch it. And CallInst part does not handle scalable vectors so no mask. And I don't think we even can assume mask is always the last operand even if so is the case for FRem.

llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll

370

replace-intrinsics-with-veclib-sleef-scalable.ll is alphabetically ordered for intrinsics tests but replace-intrinsics-with-veclib-armpl.ll is not. It starts with cos and the next one is sin. The remainder of the test file is fairly alphabetical even if log10 comes after log2. But I moved the frem tests so they come in alphabetical order as they are listed alphabetically in VecFuncs.def. I renamed them also to llvm_frem even if they not intrinsics.

[jolanta.jensen]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

148–150

Forgot about this one. Will be included in next patch.

[jolanta.jensen]

Mask fix.

[jolanta.jensen]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

148–150

Removed the unnecessary debug print.

[mgabka]

llvm/include/llvm/Analysis/VectorUtils.h

184 ↗

(On Diff #548551)

please add documentation for the new added parameter.

in my opinion the interface will be better if we use it for both scalable and fixed vectors, otherwise it is confusing what this parameter is and when it is used.

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

55

this may fail, not all instructions used here will be returning Vector.
You need the EC only for frem case to create Type for the mask, but in this case you have guaranteed that it returns a vector type. Please change it.

57

nod needed change

73

FWIW this you already refactored the code quite a bit so removing not needed bit is not a problem as part of this work.

74

The whole idea about std::optional is that the value may not exist, https://en.cppreference.com/w/cpp/utility/optional/value
and you need to handle it otherwise you will get exception.

75

IMO this dbg message is not needed as it is too detailed. LLVM tends to have more high level dbg output otherwise the dbg log are growing too quickly.

for this pass having a dbg messages to show that the is replacement and what has been replaced with what in my opinion is enough.

91–110

The final goal is to make this pass work for scalable vectors, so introducing unnecessary if/else blocks to just remove them later in my opinion is not a good idea.
Since we now want to use getParamIndexForOptionalMask we will now the place of mask.

llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-armpl.ll

192

a comment explaining why the transformation is not happening would be useful here.

[jolanta.jensen]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

55

Change to what? I was sending ElementCount as an argument but this was disliked and I was asked to retrieve it here. How do you want it? Could you explain a bit more?

74

In my opinion the check on line 74 is enough but I can add one more check for the position of the mask.

91–110

It's not only about the mask. CallInst needs it's OpBundles as well. But sure, if we add support for scalable vectors for CI it may be rewritten. But we are not there yet.

[jolanta.jensen]

Addressing review comments.

[jolanta.jensen]

llvm/include/llvm/Analysis/VectorUtils.h

184 ↗

(On Diff #548551)

Updated the documentation.
For fixed vectors do not need to retrieve the Vectorization Factor. The Module parameter is only used for scalable vectors to retrieve the Vectorization Factor.
But I realized the EC parameter can be misused if we use it with a CallInst and there is no Function in the Module. Any suggestions how to mitigate?

// 1. We don't accept a zero lanes vectorization factor.
// 2. We don't accept the demangling if the vector function is not
// present in the module unless we handle an Instruction.
if (VF == 0)
  return std::nullopt;
if (!EC && !M.getFunction(VectorName))
  return std::nullopt;

llvm/lib/Analysis/VFABIDemangling.cpp

455 ↗

(On Diff #548551)

Here, the EC parameter can be misused if it is set for a CallInst but the VectorFunction is not present in the Module. Any advice how to mitigate?

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

55

Sending NumElements as argument again, this time as optional one. Please check if this is a better solution.

57

Added a blank line again. Same with line 68.

73

Removed the assert checking for the function type. i.e. on line 62.

74

Added a check if we got a return value.

75

Removed.

llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-armpl.ll

192

Fixed.

[jolanta.jensen]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

44

This will malfunction if false is sent as an argument. To be corrected.

[jolanta.jensen]

Bugfix to a bug introduced in previous patch.

[jolanta.jensen]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

44

Now fixed.

llvm/include/llvm/Analysis/VecFuncs.def

 TLI_DEFINE_VECFUNC("exp2", "_ZGVsMxv_exp2",  SCALABLE(2), MASKED)
 TLI_DEFINE_VECFUNC("exp2f", "_ZGVsMxv_exp2f", SCALABLE(4), MASKED)
 TLI_DEFINE_VECFUNC("llvm.exp2.f64", "_ZGVsMxv_exp2", SCALABLE(2), MASKED)
 TLI_DEFINE_VECFUNC("llvm.exp2.f32", "_ZGVsMxv_exp2f", SCALABLE(4), MASKED)
 
 TLI_DEFINE_VECFUNC("exp10", "_ZGVsMxv_exp10",  SCALABLE(2), MASKED)
 TLI_DEFINE_VECFUNC("exp10f", "_ZGVsMxv_exp10f", SCALABLE(4), MASKED)
 
+TLI_DEFINE_VECFUNC("fmod", "_ZGVsMxvv_fmod", SCALABLE(2), MASKED)
+TLI_DEFINE_VECFUNC("fmodf", "_ZGVsMxvv_fmodf", SCALABLE(4), MASKED)
+
 TLI_DEFINE_VECFUNC("lgamma", "_ZGVsMxv_lgamma",  SCALABLE(2), MASKED)
 TLI_DEFINE_VECFUNC("lgammaf", "_ZGVsMxv_lgammaf", SCALABLE(4), MASKED)
 
 TLI_DEFINE_VECFUNC("log", "_ZGVsMxv_log",  SCALABLE(2), MASKED)
 TLI_DEFINE_VECFUNC("logf", "_ZGVsMxv_logf", SCALABLE(4), MASKED)
 TLI_DEFINE_VECFUNC("llvm.log.f64", "_ZGVsMxv_log", SCALABLE(2), MASKED)
 TLI_DEFINE_VECFUNC("llvm.log.f32", "_ZGVsMxv_logf", SCALABLE(4), MASKED)
 

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

STATISTIC(NumCallsReplaced,
           "Number of calls to intrinsics that have been replaced.");
 
 STATISTIC(NumTLIFuncDeclAdded,
           "Number of vector library function declarations added.");
 
 STATISTIC(NumFuncUsedAdded,
           "Number of functions added to `llvm.compiler.used`");
 
-static bool replaceWithTLIFunction(CallInst &CI, const StringRef TLIName) {
-  Module *M = CI.getModule();
-
-  Function *OldFunc = CI.getCalledFunction();
+static bool replaceWithTLIFunction(Instruction &I, const StringRef TLIName,
+                                   ElementCount *NumElements = nullptr,
+                                   bool Masked = false) {
+  Module *M = I.getModule();
+  IRBuilder<> IRBuilder(&I);
+  CallInst *CI = dyn_cast<CallInst>(&I);
 
   // Check if the vector library function is already declared in this module,
   // otherwise insert it.
   Function *TLIFunc = M->getFunction(TLIName);
+  std::string OldName;
   if (!TLIFunc) {
-    TLIFunc = Function::Create(OldFunc->getFunctionType(),
+    if (!CI) {
+      // FRem handling.
+      assert(I.getOpcode() == Instruction::FRem &&
+             "Must be a FRem instruction.");
+      assert(NumElements != nullptr && "Vectorization factor is missing.");
+      OldName = I.getOpcodeName();
+      Type *RetTy = I.getType();
+      SmallVector<Type *> Tys = {RetTy, RetTy};
+      if (Masked)
+        Tys.push_back(ToVectorTy(IRBuilder.getInt1Ty(), *NumElements));
+      TLIFunc = Function::Create(FunctionType::get(RetTy, Tys, false),
                                  Function::ExternalLinkage, TLIName, *M);
+    } else {
+      // Intrinsics handling.
+      Function *OldFunc = CI->getCalledFunction();
+      FunctionType *OldFuncTy = OldFunc->getFunctionType();
+      OldName = OldFunc->getName();
+      TLIFunc =
+          Function::Create(OldFuncTy, Function::ExternalLinkage, TLIName, *M);
       TLIFunc->copyAttributesFrom(OldFunc);
-
+      assert(OldFuncTy == TLIFunc->getFunctionType() &&
+             "Expecting function types to be identical");
+    }
     LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Added vector library function `"
                       << TLIName << "` of type `" << *(TLIFunc->getType())
                       << "` to module.\n");
-
     ++NumTLIFuncDeclAdded;
 
     // Add the freshly created function to llvm.compiler.used,
     // similar to as it is done in InjectTLIMappings
     appendToCompilerUsed(*M, {TLIFunc});
 
     LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Adding `" << TLIName
                       << "` to `@llvm.compiler.used`.\n");
     ++NumFuncUsedAdded;
   }
-
-  // Replace the call to the vector intrinsic with a call
+  // Replace the call to the frem instruction/vector intrinsic with a call
   // to the corresponding function from the vector library.
-  IRBuilder<> IRBuilder(&CI);
-  SmallVector<Value *> Args(CI.args());
+  CallInst *Replacement = nullptr;
+  if (!CI) {
+    // FRem handling.
+    SmallVector<Value *> Args(I.operand_values());
+    if (Masked) {
+      Value *AllActiveMask = IRBuilder.getAllOnesMask(*NumElements);
+      Args.push_back(AllActiveMask);
+    }
+    Replacement = IRBuilder.CreateCall(TLIFunc, Args);
+  } else {
+    // Intrinsics handling.
+    SmallVector<Value *> Args(CI->args());
     // Preserve the operand bundles.
     SmallVector<OperandBundleDef, 1> OpBundles;
-  CI.getOperandBundlesAsDefs(OpBundles);
-  CallInst *Replacement = IRBuilder.CreateCall(TLIFunc, Args, OpBundles);
-  assert(OldFunc->getFunctionType() == TLIFunc->getFunctionType() &&
-         "Expecting function types to be identical");
-  CI.replaceAllUsesWith(Replacement);
-  if (isa<FPMathOperator>(Replacement)) {
-    // Preserve fast math flags for FP math.
-    Replacement->copyFastMathFlags(&CI);
+    CI->getOperandBundlesAsDefs(OpBundles);
+    Replacement = IRBuilder.CreateCall(TLIFunc, Args, OpBundles);
   }
-
-  LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Replaced call to `"
-                    << OldFunc->getName() << "` with call to `" << TLIName
-                    << "`.\n");
+  I.replaceAllUsesWith(Replacement);
+  // Preserve fast math flags for FP math.
+  if (isa<FPMathOperator>(Replacement))
+    Replacement->copyFastMathFlags(&I);
+  LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Replaced call to `" << OldName
+                    << "` with call to `" << TLIName << "`.\n");
   ++NumCallsReplaced;
   return true;
 }
 
-static bool replaceWithCallToVeclib(const TargetLibraryInfo &TLI,
-                                    CallInst &CI) {
-  if (!CI.getCalledFunction()) {
-    return false;
-  }
+static bool replaceFremWithCallToVeclib(const TargetLibraryInfo &TLI,
+                                        Instruction &I) {
+  auto *VectorArgTy = dyn_cast<ScalableVectorType>(I.getType());
+  // We have TLI mappings for FRem on scalable vectors only.
+  if (!VectorArgTy)
+    return false;
+  ElementCount NumElements = VectorArgTy->getElementCount();
+  Type *ElementType = VectorArgTy->getElementType();
+  StringRef ScalarName;
+  if (ElementType->isFloatTy())
+    ScalarName = TLI.getName(LibFunc_fmodf);
+  else if (ElementType->isDoubleTy())
+    ScalarName = TLI.getName(LibFunc_fmod);
+  else
+    return false;
+  LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Looking up TLI mapping for `"
+                    << ScalarName << "` and vector width " << NumElements
+                    << ".\n");
+  std::string TLIName =
+      std::string(TLI.getVectorizedFunction(ScalarName, NumElements));
+  if (!TLIName.empty()) {
+    LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Found unmasked TLI function `"
+                      << TLIName << "`.\n");
+    return replaceWithTLIFunction(I, TLIName, &NumElements);
+  }
+  TLIName =
+      std::string(TLI.getVectorizedFunction(ScalarName, NumElements, true));
+  if (!TLIName.empty()) {
+    LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Found masked TLI function `"
+                      << TLIName << "`.\n");
+    return replaceWithTLIFunction(I, TLIName, &NumElements, true);
+  }
+  LLVM_DEBUG(dbgs() << DEBUG_TYPE
+                    << ": Did not find suitable vectorized version of `"
+                    << ScalarName << "`.\n");
 
-  auto IntrinsicID = CI.getCalledFunction()->getIntrinsicID();
+static bool replaceWithCallToVeclib(const TargetLibraryInfo &TLI,
+                                    Instruction &I) {
+
+  // Replacement can be performed for FRem instruction.
+  if (I.getOpcode() == Instruction::FRem)
+    return replaceFremWithCallToVeclib(TLI, I);
+
+  CallInst *CI = dyn_cast<CallInst>(&I);
+  if (!CI || !CI->getCalledFunction())
+    return false;
+
+  auto IntrinsicID = CI->getCalledFunction()->getIntrinsicID();
   if (IntrinsicID == Intrinsic::not_intrinsic) {
     // Replacement is only performed for intrinsic functions
     return false;
   }
 
   // Convert vector arguments to scalar type and check that
   // all vector operands have identical vector width.
   ElementCount VF = ElementCount::getFixed(0);
   SmallVector<Type *> ScalarTypes;
-  for (auto Arg : enumerate(CI.args())) {
+  for (auto Arg : enumerate(CI->args())) {
     auto *ArgType = Arg.value()->getType();
     // Vector calls to intrinsics can still have
     // scalar operands for specific arguments.
     if (isVectorIntrinsicWithScalarOpAtArg(IntrinsicID, Arg.index())) {
       ScalarTypes.push_back(ArgType);
     } else {
       // The argument in this place should be a vector if
       // this is a call to a vector intrinsic.
for (auto Arg : enumerate(CI->args())) {
     }
   }
 
   // Try to reconstruct the name for the scalar version of this
   // intrinsic using the intrinsic ID and the argument types
   // converted to scalar above.
   std::string ScalarName;
   if (Intrinsic::isOverloaded(IntrinsicID)) {
-    ScalarName = Intrinsic::getName(IntrinsicID, ScalarTypes, CI.getModule());
+    ScalarName = Intrinsic::getName(IntrinsicID, ScalarTypes, CI->getModule());
   } else {
     ScalarName = Intrinsic::getName(IntrinsicID).str();
   }
 
   if (!TLI.isFunctionVectorizable(ScalarName)) {
     // The TargetLibraryInfo does not contain a vectorized version of
     // the scalar function.
     return false;
LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Looking up TLI mapping for `"
                     << ScalarName << "` and vector width " << VF << ".\n");
 
   if (!TLIName.empty()) {
     // Found the correct mapping in the TargetLibraryInfo,
     // replace the call to the intrinsic with a call to
     // the vector library function.
     LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Found TLI function `" << TLIName
                       << "`.\n");
-    return replaceWithTLIFunction(CI, TLIName);
+    return replaceWithTLIFunction(*CI, TLIName);
   }
 
   return false;
 }
 
 static bool runImpl(const TargetLibraryInfo &TLI, Function &F) {
   bool Changed = false;
-  SmallVector<CallInst *> ReplacedCalls;
+  SmallVector<Instruction *> ReplacedCalls;
   for (auto &I : instructions(F)) {
-    if (auto *CI = dyn_cast<CallInst>(&I)) {
-      if (replaceWithCallToVeclib(TLI, *CI)) {
-        ReplacedCalls.push_back(CI);
-        Changed = true;
-      }
-    }
-  }
-  // Erase the calls to the intrinsics that have been replaced
-  // with calls to the vector library.
-  for (auto *CI : ReplacedCalls) {
-    CI->eraseFromParent();
-  }
+    if (replaceWithCallToVeclib(TLI, I)) {
+      ReplacedCalls.push_back(&I);
+  // Erase the calls to the intrinsics and the frem instructions that have been
+  // replaced with calls to the vector library.
+  for (auto *I : ReplacedCalls)
+    I->eraseFromParent();
+
   return Changed;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // New pass manager implementation.
 ////////////////////////////////////////////////////////////////////////////////
 PreservedAnalyses ReplaceWithVeclib::run(Function &F,
                                          FunctionAnalysisManager &AM) {
   const TargetLibraryInfo &TLI = AM.getResult<TargetLibraryAnalysis>(F);
   auto Changed = runImpl(TLI, F);
   if (Changed) {
     PreservedAnalyses PA;
     PA.preserveSet<CFGAnalyses>();
     PA.preserve<TargetLibraryAnalysis>();
     PA.preserve<ScalarEvolutionAnalysis>();
     PA.preserve<LoopAccessAnalysis>();
     PA.preserve<DemandedBitsAnalysis>();
     PA.preserve<OptimizationRemarkEmitterAnalysis>();
     return PA;
   } else {
     // The pass did not replace any calls, hence it preserves all analyses.
     return PreservedAnalyses::all();
   }

llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-armpl.ll

 ; CHECK-SAME: (<vscale x 4 x float> [[IN:%.*]]) #[[ATTR1]] {
 ; CHECK-NEXT:    [[TMP1:%.*]] = call fast <vscale x 4 x float> @llvm.exp2.nxv4f32(<vscale x 4 x float> [[IN]])
 ; CHECK-NEXT:    ret <vscale x 4 x float> [[TMP1]]
 ;
   %1 = call fast <vscale x 4 x float> @llvm.exp2.nxv4f32(<vscale x 4 x float> %in)
   ret <vscale x 4 x float> %1
 }
 
 
 declare <2 x double> @llvm.log.v2f64(<2 x double>)
 declare <4 x float> @llvm.log.v4f32(<4 x float>)
 declare <vscale x 2 x double> @llvm.log.nxv2f64(<vscale x 2 x double>)
 declare <vscale x 4 x float> @llvm.log.nxv4f32(<vscale x 4 x float>)
 
 define <2 x double> @llvm_log_f64(<2 x double> %in) {
 ; CHECK-LABEL: define <2 x double> @llvm_log_f64
 ; CHECK-SAME: (<2 x double> [[IN:%.*]]) {

 ; CHECK-SAME: (<vscale x 4 x float> [[IN:%.*]], <vscale x 4 x float> [[POWER:%.*]]) #[[ATTR1]] {
 ; CHECK-NEXT:    [[TMP1:%.*]] = call fast <vscale x 4 x float> @llvm.pow.nxv4f32(<vscale x 4 x float> [[IN]], <vscale x 4 x float> [[POWER]])
 ; CHECK-NEXT:    ret <vscale x 4 x float> [[TMP1]]
 ;
   %1 = call fast <vscale x 4 x float> @llvm.pow.nxv4f32(<vscale x 4 x float> %in, <vscale x 4 x float> %power)
   ret <vscale x 4 x float> %1
 }
 
+; NOTE: TLI mappings for FREM instruction.
+
+define <vscale x 2 x double> @frem_vscale_f64(<vscale x 2 x double> %in1, <vscale x 2 x double> %in2) #0 {
+; CHECK-LABEL: define <vscale x 2 x double> @frem_vscale_f64
+; CHECK-SAME: (<vscale x 2 x double> [[IN1:%.*]], <vscale x 2 x double> [[IN2:%.*]]) #[[ATTR1]] {
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast <vscale x 2 x double> @armpl_svfmod_f64_x(<vscale x 2 x double> [[IN1]], <vscale x 2 x double> [[IN2]], <vscale x 2 x i1> shufflevector (<vscale x 2 x i1> insertelement (<vscale x 2 x i1> poison, i1 true, i64 0), <vscale x 2 x i1> poison, <vscale x 2 x i32> zeroinitializer))
+; CHECK-NEXT:    ret <vscale x 2 x double> [[TMP1]]
+;
+  %out = frem fast <vscale x 2 x double> %in1, %in2
+  ret <vscale x 2 x double> %out
+}
+
+define <vscale x 4 x float> @frem_vscale_f32(<vscale x 4 x float> %in1, <vscale x 4 x float> %in2)  #0 {
+; CHECK-LABEL: define <vscale x 4 x float> @frem_vscale_f32
+; CHECK-SAME: (<vscale x 4 x float> [[IN1:%.*]], <vscale x 4 x float> [[IN2:%.*]]) #[[ATTR1]] {
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast <vscale x 4 x float> @armpl_svfmod_f32_x(<vscale x 4 x float> [[IN1]], <vscale x 4 x float> [[IN2]], <vscale x 4 x i1> shufflevector (<vscale x 4 x i1> insertelement (<vscale x 4 x i1> poison, i1 true, i64 0), <vscale x 4 x i1> poison, <vscale x 4 x i32> zeroinitializer))
+; CHECK-NEXT:    ret <vscale x 4 x float> [[TMP1]]
+;
+  %out = frem fast <vscale x 4 x float> %in1, %in2
+  ret <vscale x 4 x float> %out
+}
+
 attributes #0 = { "target-features"="+sve" }

llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll

 ; CHECK-LABEL: @llvm_trunc_vscale_f32(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call fast <vscale x 4 x float> @llvm.trunc.nxv4f32(<vscale x 4 x float> [[IN:%.*]])
 ; CHECK-NEXT:    ret <vscale x 4 x float> [[TMP1]]
 ;
   %1 = call fast <vscale x 4 x float> @llvm.trunc.nxv4f32(<vscale x 4 x float> %in)
   ret <vscale x 4 x float> %1
 }
 
+; NOTE: TLI mapping for FREM instruction.
+
+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(
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast <vscale x 2 x double> @_ZGVsMxvv_fmod(<vscale x 2 x double> [[IN1:%.*]], <vscale x 2 x double> [[IN2:%.*]], <vscale x 2 x i1> shufflevector (<vscale x 2 x i1> insertelement (<vscale x 2 x i1> poison, i1 true, i64 0), <vscale x 2 x i1> poison, <vscale x 2 x i32> zeroinitializer))
+; CHECK-NEXT:    ret <vscale x 2 x double> [[TMP1]]
+;
+  %out = frem fast <vscale x 2 x double> %in1, %in2
+  ret <vscale x 2 x double> %out
+}
+
+define <vscale x 4 x float> @frem_vscale_f32(<vscale x 4 x float> %in1, <vscale x 4 x float> %in2) {
+; CHECK-LABEL: @frem_vscale_f32(
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast <vscale x 4 x float> @_ZGVsMxvv_fmodf(<vscale x 4 x float> [[IN1:%.*]], <vscale x 4 x float> [[IN2:%.*]], <vscale x 4 x i1> shufflevector (<vscale x 4 x i1> insertelement (<vscale x 4 x i1> poison, i1 true, i64 0), <vscale x 4 x i1> poison, <vscale x 4 x i32> zeroinitializer))
+; CHECK-NEXT:    ret <vscale x 4 x float> [[TMP1]]
+;
+  %out = frem fast <vscale x 4 x float> %in1, %in2
+  ret <vscale x 4 x float> %out
+}
+
 declare <vscale x 2 x double> @llvm.ceil.nxv2f64(<vscale x 2 x double>)
 declare <vscale x 4 x float> @llvm.ceil.nxv4f32(<vscale x 4 x float>)
 declare <vscale x 2 x double> @llvm.copysign.nxv2f64(<vscale x 2 x double>, <vscale x 2 x double>)
 declare <vscale x 4 x float> @llvm.copysign.nxv4f32(<vscale x 4 x float>, <vscale x 4 x float>)
 declare <vscale x 2 x double> @llvm.cos.nxv2f64(<vscale x 2 x double>)
 declare <vscale x 4 x float> @llvm.cos.nxv4f32(<vscale x 4 x float>)
 declare <vscale x 2 x double> @llvm.exp.nxv2f64(<vscale x 2 x double>)
 declare <vscale x 4 x float> @llvm.exp.nxv4f32(<vscale x 4 x float>)

llvm/test/Transforms/LoopVectorize/AArch64/sleef-calls-aarch64.ll

 ; 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
 
 target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
 target triple = "aarch64-unknown-linux-gnu"
 
 declare double @acos(double) #0
 declare float @acosf(float) #0
 declare double @llvm.acos.f64(double) #0
 declare float @llvm.acos.f32(float) #0
define void @exp10_f32(float* nocapture %varray) {
   %iv.next = add nuw nsw i64 %iv, 1
   %exitcond = icmp eq i64 %iv.next, 1000
   br i1 %exitcond, label %for.end, label %for.body
 
   for.end:
   ret void
 }
 
+declare double @fmod(double, double) #0
+declare float @fmodf(float, float) #0
+
+define void @fmod_f64(double* nocapture %varray) {
+  ; CHECK-LABEL: @fmod_f64(
+  ; SVE:      [[TMP5:%.*]] = call <vscale x 2 x double> @_ZGVsMxvv_fmod(<vscale x 2 x double> [[TMP4:%.*]], <vscale x 2 x double> [[TMP4:%.*]], <vscale x 2 x i1> {{.*}})
+  ; CHECK:    ret void
+  ;
+  entry:
+  br label %for.body
+
+  for.body:
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %for.body ]
+  %tmp = trunc i64 %iv to i32
+  %conv = sitofp i32 %tmp to double
+  %call = tail call double @fmod(double %conv, double %conv)
+  %arrayidx = getelementptr inbounds double, double* %varray, i64 %iv
+  store double %call, double* %arrayidx, align 8
+  %iv.next = add nuw nsw i64 %iv, 1
+  %exitcond = icmp eq i64 %iv.next, 1000
+  br i1 %exitcond, label %for.end, label %for.body
+
+  for.end:
+  ret void
+}
+
+define void @fmod_f32(float* nocapture %varray) {
+  ; CHECK-LABEL: @fmod_f32(
+  ; SVE:      [[TMP5:%.*]] = call <vscale x 4 x float> @_ZGVsMxvv_fmodf(<vscale x 4 x float> [[TMP4:%.*]], <vscale x 4 x float> [[TMP4:%.*]], <vscale x 4 x i1> {{.*}})
+  ; CHECK:    ret void
+  ;
+  entry:
+  br label %for.body
+
+  for.body:
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %for.body ]
+  %tmp = trunc i64 %iv to i32
+  %conv = sitofp i32 %tmp to float
+  %call = tail call float @fmodf(float %conv, float %conv)
+  %arrayidx = getelementptr inbounds float, float* %varray, i64 %iv
+  store float %call, float* %arrayidx, align 4
+  %iv.next = add nuw nsw i64 %iv, 1
+  %exitcond = icmp eq i64 %iv.next, 1000
+  br i1 %exitcond, label %for.end, label %for.body
+
+  for.end:
+  ret void
+}
+
 declare double @lgamma(double) #0
 declare float @lgammaf(float) #0
 declare double @llvm.lgamma.f64(double) #0
 declare float @llvm.lgamma.f32(float) #0
 
 define void @lgamma_f64(double* nocapture %varray) {
   ; CHECK-LABEL: @lgamma_f64(
   ; NEON:     [[TMP5:%.*]] = call <2 x double> @_ZGVnN2v_lgamma(<2 x double> [[TMP4:%.*]])