Page MenuHomePhabricator

[Polly][Fix] Teach the IslExprBuilder about vector lanes
Needs ReviewPublic

Authored by jdoerfert on Oct 9 2014, 8:18 AM.

Details

Reviewers
simbuerg
dpeixott
Summary
We can now add an offset to an isl_id in the IslExprBuilder in order
to adjust the created code for the current vector lane.

The modified code paths in the VectorBlockGenerator are tested by the
two test cases.

Diff Detail

Event Timeline

jdoerfert updated this revision to Diff 14655.Oct 9 2014, 8:18 AM
jdoerfert retitled this revision from to [Polly][Fix] Teach the IslExprBuilder about vector lanes.
jdoerfert updated this object.
jdoerfert added subscribers: Restricted Project, Unknown Object (MLST).
jdoerfert updated this revision to Diff 14657.Oct 9 2014, 8:45 AM

Simplified the code and allowed only one Id to have an offset called vector lane

grosser edited edge metadata.Oct 10 2014, 8:17 AM

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?

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.

lib/CodeGen/IslExprBuilder.cpp
450

This seems incorrect in case the vector loop has a stride > 1. You would need to add Stride * VectorLane, I suppose.

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:

  1. We would change the map really often and that is way more expensive than what we do now.
  2. We would need to expose the IDToValue map (minor issue though).
  3. 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.

Thanks!

lib/CodeGen/IslExprBuilder.cpp
450

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.

Hi Johannes,

just looking at the proposed commit message, I am not sure about the motivation of this patch (why do we want it). Is this patch still of interest?

grosser resigned from this revision.Jun 11 2016, 3:30 AM
grosser removed a reviewer: grosser.

Hi Johannes,

it is currently unclear to me if this patch is still needed, but at least it seems to be a patch that is currently not worked on. To clean up my review list I drop this patch from my queue. Please feel free to add me back in case this patch is still needed and you find time to get back to it.

Best,
Tobias

PS: Me dropping out of outdated reviews is not a sign that I do not like the review. I acknowledge that our review process was not always optimal and as part of trying to improve it I clean my review queue.

sebpop resigned from this revision.Sep 19 2016, 1:22 PM
sebpop removed a reviewer: sebpop.