This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Third Updated translating the function Scop::foldSizeConstantsToRight() to isl C++
Needs RevisionPublic

Authored by Siddharth on Aug 23 2017, 6:08 AM.

Details

Reviewers
grosser
bollu
Summary

This one doesn't contain any other modifications except those done to Scop::foldSizeConstantsToRight()

Diff Detail

Event Timeline

Siddharth created this revision.Aug 23 2017, 6:08 AM
grosser edited edge metadata.Aug 23 2017, 7:15 AM

A lot better, some comments.

lib/Analysis/ScopInfo.cpp
3406

Why do you introduce a new variable. Just use:

isl::union_set Accesses = getAccesses.range();

3413

Same here, this can be written as:

isl::space Space = Array->getSpace();
Space = Space.align_params(Acessed.get_space());

3415

No need for "(" after the "!"

3426

This works? This should read:

int Dims = Elements.dim(isl::dim::set);

3429

This should read:

isl::set DimOnly = Elements.project_out(isl::dim::set, 0, i);

3452

Add isl:: prefix.

Also, do you need two local_space ?

3459

Does this compile?

No copy is needed here. Just write:

isl::basic_set ZeroSet = DimHull;

3489

Does this work? It should probably read:

Access->setAccessRelation(Access->getAccessRelation().apply_range(Transform);

3503

No need for a return.

3503–3504

Drop the empty line.

grosser requested changes to this revision.Aug 24 2017, 1:09 PM

Mark this as requesting changes.

This revision now requires changes to proceed.Aug 24 2017, 1:09 PM
bollu edited edge metadata.Aug 25 2017, 2:51 AM

Also, for future reference, note that an old review can be updated: There is an update diff option to the right hand side at the top of the web page. That way, we won't have multiple revisions like D37061 and D36843. This reduces the number of places someone needs to look to see the total change :). Thanks!

Siddharth updated this revision to Diff 113649.Sep 2 2017, 1:44 AM
Siddharth edited edge metadata.

Made appropriate changes as mentioned in comments in the previous patch

grosser requested changes to this revision.Sep 3 2017, 10:18 AM

Hi Siddharth,

thank you for your update. This looks already a lot better, but it seems the patch above does not yet pass 'make check-polly'. Could you possibly address the remaining bugs. Most of them should be pretty straightforward.

lib/Analysis/ScopInfo.cpp
3406

Why does what I suggested above not work? It seems you drop the _range() call? Does this pass "make check-polly"?

This revision now requires changes to proceed.Sep 3 2017, 10:18 AM
Siddharth updated this revision to Diff 114805.Sep 12 2017, 5:34 AM
Siddharth edited edge metadata.

With few more required changes. Previous errors were gone with make check-polly

grosser requested changes to this revision.Sep 19 2017, 5:39 AM

Hi Siddharth,

I just applied your patch using "arc patch D37061", but it seems to not yet compile. Did you test it locally?

Best,
Tobias

lib/Analysis/ScopInfo.cpp
3405

This line does not compile for me. I get the following error:

/home/grosser/Projects/polly/git/tools/polly/lib/Analysis/ScopInfo.cpp:3405:45: error: expected '(' for function-style cast or type construction
   isl::union_set Accessed = isl::union_set union_map::range(getAccesses.release());
This revision now requires changes to proceed.Sep 19 2017, 5:39 AM