[llvm-commits] [LLVMdev] [PATCH] BasicBlock Autovectorization Pass
tobias at grosser.es
Thu Dec 29 06:00:49 PST 2011
On 12/14/2011 01:25 AM, Hal Finkel wrote:
> I've attached an updated copy of the patch. I believe that I accounted
> for all of your suggestions except for:
> 1. You said that I could make AA a member of the class and initialize it
> for each basic block. I suppose that I'd need to make it a pointer, but
> more generally, what is the thread-safely model that I should have in
> mind for the analysis passes (will multiple threads always use distinct
> instances of the passes)? Furthermore, if I am going to make AA a class
> variable that is re-initialized on every call to runOnBasicBlock, then I
> should probably do the same with all of the top-level maps, etc. Do you
I am actually not sure about the thread safety conventions in LLVM. I
personally always assumed that a transformation class will only be
instantiated and used within a single thread and in case of multiple
threads, multiple class instances would be created.
The pattern of assigning analysis results to object variables is common
(e.g. lib/Analysis/IVUsers.cpp). So we should be able to use it. I
personally do not have any strong opinion about the other top-level
maps. Just decide yourself what is easier to read.
> 2. I have not (yet) changed the types of the maps from holding Value* to
> Instruction*. Doing so would eliminate a few casts, but would cause
> even-more casts (and maybe checks) in computePairsConnectedTo. Even so,
> it might be worthwhile to make the change for clarity (conceptually,
> those maps do hold only pointers to instructions).
I believe if code relies on the assumption that some data structures
only objects of a certain type, we should use that type to define the
data structures. Though, I do not think this change is required to
commit the patch. It can be addressed after the code was committed.
One thing that I would still like to have is a test case where
bb-vectorize-search-limit is needed to avoid exponential compile time
growth and another test case that is not optimized, if
bb-vectorize-search-limit is set to a value less than 4000. I think
those cases are very valuable to understand the performance behavior of
this code. Especially, as I am not yet sure why we need a value as high
This is my last issue on this patch, that should be addressed before
committing it. After we discussed this, I propose to post a final patch
stating that you will commit after three days to give other people a
chance to veto.
More information about the llvm-commits