[llvm-dev] 6 separate instances of static getPointerOperand(). Time to consolidate?
Saito, Hideki via llvm-dev
llvm-dev at lists.llvm.org
Fri Feb 9 16:27:45 PST 2018
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
-----Original Message-----
From: Saito, Hideki
Sent: Tuesday, February 06, 2018 11:31 AM
To: Justin Bogner <mail at justinbogner.com>; Saito, Hideki via llvm-dev <llvm-dev at lists.llvm.org>
Subject: RE: [llvm-dev] 6 separate instances of static getPointerOperand(). Time to consolidate?
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
-------------------------
// FIXME: The following helper functions have multiple implementations
// in the project. They can be effectively organized in a common Load/Store
// utilities unit.
/// A helper function that returns the pointer operand of a load or store
/// instruction.
static Value *getPointerOperand(Value *I) {
if (auto *LI = dyn_cast<LoadInst>(I))
return LI->getPointerOperand();
if (auto *SI = dyn_cast<StoreInst>(I))
return SI->getPointerOperand();
return nullptr;
}
/// A helper function that returns the type of loaded or stored value.
static Type *getMemInstValueType(Value *I) {
assert((isa<LoadInst>(I) || isa<StoreInst>(I)) &&
"Expected Load or Store instruction");
if (auto *LI = dyn_cast<LoadInst>(I))
return LI->getType();
return cast<StoreInst>(I)->getValueOperand()->getType();
}
/// A helper function that returns the alignment of load or store instruction.
static unsigned getMemInstAlignment(Value *I) {
assert((isa<LoadInst>(I) || isa<StoreInst>(I)) &&
"Expected Load or Store instruction");
if (auto *LI = dyn_cast<LoadInst>(I))
return LI->getAlignment();
return cast<StoreInst>(I)->getAlignment();
}
/// A helper function that returns the address space of the pointer operand of
/// load or store instruction.
static unsigned getMemInstAddressSpace(Value *I) {
assert((isa<LoadInst>(I) || isa<StoreInst>(I)) &&
"Expected Load or Store instruction");
if (auto *LI = dyn_cast<LoadInst>(I))
return LI->getPointerAddressSpace();
return cast<StoreInst>(I)->getPointerAddressSpace();
}
-----Original Message-----
From: Justin Bogner [mailto:justin at justinbogner.com] On Behalf Of Justin Bogner
Sent: Tuesday, February 06, 2018 10:58 AM
To: Saito, Hideki via llvm-dev <llvm-dev at lists.llvm.org>
Cc: Saito, Hideki <hideki.saito at intel.com>
Subject: Re: [llvm-dev] 6 separate instances of static getPointerOperand(). Time to consolidate?
"Saito, Hideki via llvm-dev" <llvm-dev at 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.
> Objections? Alternatives?
>
> Thanks,
> Hideki
> -----------
> Relevant background info
> http://lists.llvm.org/pipermail/llvm-dev/2018-January/120164.html
> if anyone is curious about my cleanup work.
>
> ------------- Analysis/Delinearization.cpp
>
> static Value *getPointerOperand(Instruction &Inst) {
> if (LoadInst *Load = dyn_cast<LoadInst>(&Inst))
> return Load->getPointerOperand();
> else if (StoreInst *Store = dyn_cast<StoreInst>(&Inst))
> return Store->getPointerOperand();
> else if (GetElementPtrInst *Gep = dyn_cast<GetElementPtrInst>(&Inst))
> return Gep->getPointerOperand();
> return nullptr;
> }
>
> ------------- Analysis/DependenceAnalysis.cpp
>
> static
> Value *getPointerOperand(Instruction *I) {
> if (LoadInst *LI = dyn_cast<LoadInst>(I))
> return LI->getPointerOperand();
> if (StoreInst *SI = dyn_cast<StoreInst>(I))
> return SI->getPointerOperand();
> llvm_unreachable("Value is not load or store instruction");
> return nullptr;
> }
>
> ------------- Analysis/LoopAccessAnalysis.cpp
>
> /// Take the pointer operand from the Load/Store instruction.
> /// Returns NULL if this is not a valid Load/Store instruction.
> static Value *getPointerOperand(Value *I) {
> if (auto *LI = dyn_cast<LoadInst>(I))
> return LI->getPointerOperand();
> if (auto *SI = dyn_cast<StoreInst>(I))
> return SI->getPointerOperand();
> return nullptr;
> }
>
> ------------ Transforms/Scalar/EarlyCSE.cpp class ParseMemoryInst
>
> Value *getPointerOperand() const {
> if (IsTargetMemInst) return Info.PtrVal;
> if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
> return LI->getPointerOperand();
> } else if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
> return SI->getPointerOperand();
> }
> return nullptr;
> }
>
> ------------ Transforms/Vectorize/LoadStoreVectorizer.cpp class Vectorizer
>
> Value *Vectorizer::getPointerOperand(Value *I) const {
> if (LoadInst *LI = dyn_cast<LoadInst>(I))
> return LI->getPointerOperand();
> if (StoreInst *SI = dyn_cast<StoreInst>(I))
> return SI->getPointerOperand();
> return nullptr;
> }
>
> ---------- Transforms/Vectorize/LoopVectorize.cpp
>
> // FIXME: The following helper functions have multiple implementations
> // in the project. They can be effectively organized in a common
> Load/Store // utilities unit.
> /// A helper function that returns the pointer operand of a load or
> store /// instruction.
> static Value *getPointerOperand(Value *I) {
> if (auto *LI = dyn_cast<LoadInst>(I))
> return LI->getPointerOperand();
> if (auto *SI = dyn_cast<StoreInst>(I))
> return SI->getPointerOperand();
> return nullptr;
> }
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
More information about the llvm-dev
mailing list