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

Johannes Doerfert doerfert at cs.uni-saarland.de
Fri Oct 10 08:33:52 PDT 2014

>>! 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.
  # We would need to expose the IDToValue map (minor issue though).
  # 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.

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

> I did not fully think this trough in terms of code, but wanted to get the review out quick as the bug I found may save you some time. Please let me know what you think regarding the comment above.

Comment at: lib/CodeGen/IslExprBuilder.cpp:450
@@ +449,3 @@
+    Value *Offset = ConstantInt::get(V->getType(), VectorLane);
+    V = Builder.CreateAdd(V, Offset);
+  }
grosser wrote:
> This seems incorrect in case the vector loop has a stride > 1. You would need to add Stride * VectorLane, I suppose.
Correct. That might even be the problem I have in some of my privatization lnt tests ;)

I could correct his by adding a second field with the stride, however you do not think this is the right way to go anyway.


More information about the llvm-commits mailing list