Move the 2 classes out of LoopVectorize.cpp to make it easier to re-use
them for VPlan outside LoopVectorize.cpp
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I like the idea of the interleave analysis to be at a higher ground, but we have to be careful with LV-specific logic and only hoist what it truly generic.
Some comments inline.
include/llvm/Analysis/VectorUtils.h | ||
---|---|---|
510 ↗ | (On Diff #156075) | This sounds bad. If they have common signature we could have linking issues. Regardless, we should keep inline functions to cpp files, not headers. |
lib/Analysis/VectorUtils.cpp | ||
587 ↗ | (On Diff #156075) | This is not strictly true for all users, which are many. $ grep -riI VectorUtils.h lib/Transforms/InstCombine/InstCombineVectorOps.cpp:#include "llvm/Analysis/VectorUtils.h" lib/Transforms/Scalar/Scalarizer.cpp:#include "llvm/Analysis/VectorUtils.h" lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:#include "llvm/Analysis/VectorUtils.h" lib/Transforms/Vectorize/SLPVectorizer.cpp:#include "llvm/Analysis/VectorUtils.h" lib/Transforms/Vectorize/LoopVectorize.cpp:#include "llvm/Analysis/VectorUtils.h" lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:#include "llvm/Analysis/VectorUtils.h" lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:#include "llvm/Analysis/VectorUtils.h" lib/Analysis/InstructionSimplify.cpp:#include "llvm/Analysis/VectorUtils.h" lib/Analysis/VectorUtils.cpp:#include "llvm/Analysis/VectorUtils.h" lib/Analysis/LoopAccessAnalysis.cpp:#include "llvm/Analysis/VectorUtils.h" lib/Target/X86/X86InterleavedAccess.cpp:#include "llvm/Analysis/VectorUtils.h" lib/Target/ARM/ARMISelLowering.cpp:#include "llvm/Analysis/VectorUtils.h" lib/Target/AArch64/AArch64ISelLowering.cpp:#include "llvm/Analysis/VectorUtils.h" include/llvm/Analysis/TargetTransformInfoImpl.h:#include "llvm/Analysis/VectorUtils.h" |
Thanks for having a look so quickly! Maybe there is a better place to put it, maybe we should keep it local to lib/Transforms/Vectorize/? I initially put it in VectorUtils.h, because I wanted to avoid creating unnecessary new files (and splitting things up unnecessarily), but given that VectorUtils.h is used in quite a few places, I am happy to put it wherever it would fit best :)
include/llvm/Analysis/VectorUtils.h | ||
---|---|---|
510 ↗ | (On Diff #156075) | Yep I did some digging but could not really pin down other users where they would be helpful. As indicated in the comment, neither LoopVectorize.cpp nor VectorUtils.h seems like appropriate places. However I am unsure what the ideal place would be? |
The location of the files is not the problem, my comment was only about:
#define LV_NAME "loop-vectorize" #define DEBUG_TYPE LV_NAME
which, as you move the code from LV to Analysis, and other code include that, the methods called by other code will still print LV debug traces and confuse developers debugging the compiler.
include/llvm/Analysis/VectorUtils.h | ||
---|---|---|
510 ↗ | (On Diff #156075) | getMemInstValueType is only ever used in LoopVectorize.cpp, so could stay there until we find a better place. The other two, with your patch, seem to be used on both LoopVectorize.cpp and VectorUtils.cpp and they're not even in the same directory. But looking at what they are, there's absolutely nothing LV about them, they're just Value helpers, which could easily be transferred to LoadInst/StoreInst directly, as a member function. |
Moved getMemInstAlignment and getMemInstAddressSpace to IR/Instructions.h which already contains similar helpers. Should I rename them to getLoadStoreAlignment and getLoadStoreAddressSpace to be more in line with the existing getLoadStorePointerOperand?
Also changed the DEBUG_TYPE to 'vectorutils'
Moved getMemInstAlignment and getMemInstAddressSpace to IR/Instructions.h which already contains similar helpers. Should I rename them to getLoadStoreAlignment and getLoadStoreAddressSpace to be more in line with the existing getLoadStorePointerOperand?
Sounds good, thanks!
--renato
include/llvm/Analysis/VectorUtils.h | ||
---|---|---|
263 ↗ | (On Diff #161742) | Do we need count? if (Members.find(Key) == Members.end()) return nullptr; ? |
include/llvm/Analysis/VectorUtils.h | ||
---|---|---|
263 ↗ | (On Diff #161742) | No need to count here I think, but I would prefer to keep this change NFC. Feel free to fix this in the the current version in LoopVectorize.cpp. Otherwise I can make the count() change separately too. Before submitting the patch, I will make sure it contains any changes made the LoopVectorize.cpp's version of the code. |
include/llvm/Analysis/VectorUtils.h | ||
---|---|---|
263 ↗ | (On Diff #161742) |
Thanks! I've rebased the patch and will wait for a bit, in case there are any additional concerns, please let me know.