[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:53
+      CI ? ElementCount::getFixed(0)
+         : (dyn_cast<ScalableVectorType>(I.getType()))->getElementCount();
   if (!TLIFunc) {
----------------
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?


================
Comment at: llvm/lib/CodeGen/ReplaceWithVeclib.cpp:76
+          return false;
+        unsigned MaskPos = Info->getParamIndexForOptionalMask().value();
+        LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Mask position `" << MaskPos
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156439