6 separate instances of static getPointerOperand(). Time to consolidate?

LLVM friends,

I’m currently trying to make LoopVectorizationLegality class in Transform/Vectorize/LoopVectorize.cpp

more modular and eventually move it to Analysis directory tree. It uses several file scope helper functions

that do not really belong to LoopVectorize. Let me start from getPointerOperand(). Within LLVM, there are

five other similar functions defined.

I think it’s time to define/declare this function in one place.

Define in include/llvm/IR/Instructions.h as an inline function?

Declare in include/llvm/IR/Instructions.h and define in lib/IR/Instructions.cpp?

New files? (e.g., IR/InstructionUtils.h and .cpp?)

Other suggesctions?

I suggest taking the implementation from Analysis/Delinearization.cpp, and create

IR/InstructionUtils.h and .cpp. They can then be used to consolidate other

Instruction helper functions to one place.

Objections? Alternatives?

Thanks,

Hideki

"Saito, Hideki via llvm-dev" <llvm-dev@lists.llvm.org> writes:

LLVM friends,

I'm currently trying to make LoopVectorizationLegality class in
Transform/Vectorize/LoopVectorize.cpp more modular and eventually move
it to Analysis directory tree. It uses several file scope helper
functions that do not really belong to LoopVectorize. Let me start
from getPointerOperand(). Within LLVM, there are five other similar
functions defined.

I think it's time to define/declare this function in one place.

Define in include/llvm/IR/Instructions.h as an inline function?
Declare in include/llvm/IR/Instructions.h and define in lib/IR/Instructions.cpp?
New files? (e.g., IR/InstructionUtils.h and .cpp?)
               Other suggesctions?

I suggest taking the implementation from Analysis/Delinearization.cpp,
and create IR/InstructionUtils.h and .cpp. They can then be used to
consolidate other Instruction helper functions to one place.

What other instruction helper functions do you have in mind? Names like
"Utils" worry me because they generally turn into a dumping ground for
things that people didn't bother to think about where belong, which in
turn makes organizing those things later difficult.

What LoopVectorize.cpp has are the following. Each function may have to have a separate consolidation discussion.
I'm bringing up getpointerOperand() since I actually found multiple instances defined/used.
DependenceAnalysis.cpp has isLoadOrStore(). LoopAccessAnalysis.cpp has getAddressSpaceOperand().
I'm sure there are others that might be worth discussing within this thread or a follow up. File name
could be IR/LoadStoreGEPUtils.h/.cpp since they seem to be addressing similar aspects of those instructions.
I'm also fine for being very selective and use Instructions.h/.cpp to house them. I brought up Utils
idea since I envision more than getPointerOperand() --- but as long as the number remains small,
the bottom of Instructions.h should work.

I'm not really a big fan of helper functions either, but I suspect that trying to add those to member functions
of Instruction base class has a much higher hurdle and that's probably the reason why so many different people
(those who wrote the code and those who have reviewed) opted for the helper function approach.

Thanks,
Hideki

It looks like consolidating just getPointerOperand() alone satisfies my immediate needs. Will run tests and then upload a patch
to Phabricator in few days. For the time being, I'm defining it as an inline function at the end of Instructions.h. If you have objections,
please give me constructive feedback through code review.

In my local workspace, I was able to move LoopVectorizeHints, LoopVectorizationRequirements, and LoopVectorizationLegality
classes into LoopVectorizationLegality.h and .cpp --- from LoopVectorize.cpp. That's very promising. It still depends on
llvm/Transforms/Utils/LoopUtils.h for RecurrenceDescriptor and InductionDescriptor classes. In order for me to move
LoopVectorizationLegality.h/.cpp to Analysis tree, I need those Descriptor classes to be moved to Analysis tree also. Will
prototype that and come back.

Once LoopVectorizationLegality moves to Analysis, we should be able to use that as a utility to check whether vectorizer would consider
the loop for vectorization, for example, from some Analysis or Transform.

Thanks,
Hideki --- one small step at a time