[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/include/llvm/Analysis/VectorUtils.h:184
+tryDemangleForVFABI(StringRef MangledName, const Module &M,
+                    std::optional<ElementCount> EC = std::nullopt);
 
----------------
mgabka wrote:
> 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.
> 
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;
```


================
Comment at: llvm/lib/Analysis/VFABIDemangling.cpp:455
     return std::nullopt;
-  if (!M.getFunction(VectorName))
+  if (!EC && !M.getFunction(VectorName))
     return std::nullopt;
----------------
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? 


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:53
+      CI ? ElementCount::getFixed(0)
+         : (dyn_cast<ScalableVectorType>(I.getType()))->getElementCount();
   if (!TLIFunc) {
----------------
jolanta.jensen wrote:
> mgabka wrote:
> > 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.
> 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?
Sending NumElements as argument again, this time as optional one. Please check if this is a better solution.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:57
                       << "` to module.\n");
-
     ++NumTLIFuncDeclAdded;
----------------
mgabka wrote:
> nod needed change
Added a blank line again. Same with line 68.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:76
+          return false;
+        unsigned MaskPos = Info->getParamIndexForOptionalMask().value();
+        LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Mask position `" << MaskPos
----------------
jolanta.jensen wrote:
> mgabka wrote:
> > 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.
> In my opinion the check on line 74 is enough but I can add one more check for the position of the mask.
Added a check if we got a return value.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:77
+        unsigned MaskPos = Info->getParamIndexForOptionalMask().value();
+        LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Mask position `" << MaskPos
+                          << "`.\n");
----------------
mgabka wrote:
> 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.
Removed.


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:73
+      TLIFunc->copyAttributesFrom(OldFunc);
+      assert(OldFuncTy == TLIFunc->getFunctionType() &&
+             "Expecting function types to be identical");
----------------
mgabka wrote:
> jolanta.jensen wrote:
> > mgabka wrote:
> > > IMO  this assert is redundant,  it is essentially checking if the Function::Create works correctly, what is not needed in my opinion.
> > 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.
> FWIW this you already refactored the code quite a bit so removing not needed bit is not a problem as part of this work.
Removed the assert checking for the function type. i.e. on line 62.


================
Comment at: llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-armpl.ll:195
 
+; NOTE: TLI mappings for FREM instruction.
+
----------------
mgabka wrote:
> a comment explaining why the transformation is not happening would be useful here.
Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156439