This is an archive of the discontinued LLVM Phabricator instance.

[LV] Move InterleaveGroup and InterleavedAccessInfo to VectorUtils.h (NFC)
ClosedPublic

Authored by fhahn on Jul 18 2018, 8:18 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Jul 18 2018, 8:18 AM

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"
fhahn added a comment.Jul 18 2018, 8:49 AM

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.

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?

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.

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.

fhahn updated this revision to Diff 156301.Jul 19 2018, 10:06 AM

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

fhahn updated this revision to Diff 161742.Aug 21 2018, 9:01 AM

rebased and renamed helpers as suggested. Sorry for the delay.

xbolva00 added inline comments.
include/llvm/Analysis/VectorUtils.h
263 ↗(On Diff #161742)

Do we need count?

if (Members.find(Key) == Members.end())

return nullptr;

?

fhahn added inline comments.Aug 21 2018, 9:47 AM
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.

xbolva00 added inline comments.Aug 21 2018, 11:42 AM
include/llvm/Analysis/VectorUtils.h
263 ↗(On Diff #161742)
xbolva00 accepted this revision.Sep 11 2018, 6:10 AM

Makes sense. Please rebase before you commit this patch.

This revision is now accepted and ready to land.Sep 11 2018, 6:10 AM
fhahn updated this revision to Diff 164894.Sep 11 2018, 8:40 AM

Thanks! I've rebased the patch and will wait for a bit, in case there are any additional concerns, please let me know.

rengolin accepted this revision.Sep 11 2018, 8:58 AM

Thanks Florian! LGTM too.

This revision was automatically updated to reflect the committed changes.