[PATCH] [Polly][Fix] Teach the IslExprBuilder about vector lanes

[Johannes Doerfert]

On 10/10, Tobias Grosser wrote:
> On 10.10.2014 17:33, Johannes Doerfert wrote:
> >>>! In D5704#6, @grosser wrote:
> >>Hi Johannes,
> >>
> >>the general idea of the patch is fine, but I feel a little uncomfortable to teach the IslExprBuilder about vectorization. Vectorization logic does not really seem related to code generating an isl expression.
> >>
> >>Instead of checking for the VectorLoopId in the ExprBuilder and adjusting it there, would it make sense to (temporarily) update the IDToValueMap of the IslExprBuilder to contain the right value?
> >It would and I thought about that too. A few things are not as nice as now:
> >
> >
> >   # We would change the map really often and that is way more expensive than what we do now.
> 
> If we switch maps, that should just be a pointer update, no?
Sorry, I don't understand what you mean here. As below, do you want to
keep VectorWidth specialized maps around mapping __one__ id differently?

> >   # We would need to expose the IDToValue map (minor issue though).
> 
> We could use functions as we do it know. Alternatively, we could pass a
> second map to ExprBuilder->create() that optionally hides the content of the
> first.
> 
> I am mostly worried about not having vector specific logic ala
> 'add Vectorlane * Stride'.
I still do not see the real issue :D

Three fields in the IslExprBuilder [Id, Lane, Stride] and 3 setters are
required and we can generate generic code for any vector lane without
any hard to understand "Map-Magic".

> >   # We would need bookkeeping when we update the map to go back to the original again, overall I thing the 4 call sides would look really ugly.
> 
> If we make the map an optional argument of create() this might even simplify
> this code.
Do I understand it correctly that you want to add a second map to map
__one__ Id to a changing value, thus we update the map every vector lane
or keep VectorWidth maps of size 1 around?

> >I personally do not have a problem with the IslExprBuilder "knowing about" vector lanes, but I don't have a strong opinion on the other way either.
> >The only thing I know is that at the moment newAccessRelations + vector code generation is buggy and we need to fix it...
> 
> :-)
> 
> Thanks for working on this.
I need it to work for privatization of vector values. As you see in the
patch there are >4 different code paths a load/store can take in
VectorBlockGenerator and 4 can emit wrong code.

> Btw, have a look into createForVector(). This is the loop where we create
> the new offsets (ValueInc is the stride):
> 
> for (int i = 1; i < VectorWidth; i++)
>   IVS[i] = Builder.CreateAdd(IVS[i - 1], ValueInc, "p_vector_iv");
Saw that when I looked at the bug you found.

-- 

Johannes Doerfert
Researcher / PhD Student

Compiler Design Lab (Prof. Hack)
Saarland University, Computer Science
Building E1.3, Room 4.26

Tel. +49 (0)681 302-57521 : doerfert at cs.uni-saarland.de
Fax. +49 (0)681 302-3065  : http://www.cdl.uni-saarland.de/people/doerfert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141010/e2da4293/attachment.sig>