[PATCH] D156439: [TLI][AArch64] Extend ReplaceWithVeclib to replace vector FREM instructions for scalable vectors
Jolanta Jensen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 1 04:07:35 PDT 2023
jolanta.jensen 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);
----------------
mgabka wrote:
> 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.
Fixed. All frem handling is in replaceWithTLIFunction. I think it's better to have it all gathered there than break out into separate function(s) as it's not that much code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156439/new/
https://reviews.llvm.org/D156439
More information about the llvm-commits
mailing list