[llvm-dev] 6 separate instances of static getPointerOperand(). Time to consolidate?
Saito, Hideki via llvm-dev
llvm-dev at lists.llvm.org
Tue Feb 6 11:30:47 PST 2018
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