[PATCH] D49488: [LV] Move InterleaveGroup and InterleavedAccessInfo to VectorUtils.h (NFC)

Renato Golin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 19 06:27:37 PDT 2018


rengolin added a comment.

In https://reviews.llvm.org/D49488#1166644, @fhahn wrote:

> 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 :)


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.



================
Comment at: include/llvm/Analysis/VectorUtils.h:510
+
+// FIXME: The following helper functions have multiple implementations
+// in the project. They can be effectively organized in a common Load/Store
----------------
fhahn wrote:
> rengolin wrote:
> > This sounds bad. If they have common signature we could have linking issues.
> > 
> > Regardless, we should keep inline functions to cpp files, not headers.
> 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?
`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.



https://reviews.llvm.org/D49488





More information about the llvm-commits mailing list