[llvm-commits] [LLVMdev] [PATCH] BasicBlock Autovectorization Pass

[Tobias Grosser]

On 12/14/2011 01:25 AM, Hal Finkel wrote:
> Tobias,
>
> 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
> agree?

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 
as 4000.

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.

Tobi