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


================
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);
----------------
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);
  }
```


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:103
   }
-
   // Convert vector arguments to scalar type and check that
----------------
not needed change please revert this change.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:149
+  }
+  LLVM_DEBUG(dbgs() << DEBUG_TYPE
+                    << ": Did not find suitable vectorized version of `"
----------------
jolanta.jensen wrote:
> 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. 
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.


================
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(
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156439