[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:49
   Function *TLIFunc = M->getFunction(TLIName);
+  std::string OldName;
+  ElementCount NumElements;
----------------
mgabka wrote:
> 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.
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.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:92-111
+  CallInst *Replacement = nullptr;
+  if (CI) {
+    // Intrinsics handling.
+    SmallVector<Value *> Args(CI->args());
+    // Preserve the operand bundles.
+    SmallVector<OperandBundleDef, 1> OpBundles;
+    CI->getOperandBundlesAsDefs(OpBundles);
----------------
mgabka wrote:
> mgabka wrote:
> > 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);
> >   }
> > ```
> 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);
> ```
> 
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.


================
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:
> jolanta.jensen wrote:
> > 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.
> 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.
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156439