[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:103-114
+  if (!TLIFunc) {
+    FunctionType *FTy = nullptr;
+    Type *RetTy = I.getType();
+    if (Masked) {
+      Type *Tys[3] = {RetTy, RetTy,
+                      ToVectorTy(IRBuilder.getInt1Ty(), NumElements)};
+      FTy = FunctionType::get(RetTy, Tys, false);
----------------
jolanta.jensen wrote:
> mgabka wrote:
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156439