Page MenuHomePhabricator

[NFC] Loop Versioning for LICM code clean up

Authored by eastig on Oct 11 2016, 3:48 AM.



Clean up of the code of Loop Versioning for LICM. See comments to the changes below.

Diff Detail


Event Timeline

eastig updated this revision to Diff 74219.Oct 11 2016, 3:48 AM
eastig retitled this revision from to [NFC] Loop Versioning for LICM.
eastig updated this object.
eastig added reviewers: ashutosh.nema, anemet, sbaranga.
eastig added subscribers: llvm-commits, jmolloy.
eastig added inline comments.Oct 11 2016, 4:21 AM
161 ↗(On Diff #74219)

TargetLibraryInfo is not used in the code.

169 ↗(On Diff #74219)

Removed unused fields or made them function scoped instead of class scoped.

176 ↗(On Diff #74219)

Made internal data private.

305 ↗(On Diff #74219)

It's strange checking. It was questioned in An answer was:

Actually this is a pre-condition in LICM’s “promoteLoopAccessesToScalars” where it expects all pointers in alias should have same type.
To confirm same behaviour we added this check.

Yes, type checking is needed for AliasSet when all pointers must alias. This is done by LICM. In case of AliasSet whose pointers may alias we can have aliased pointers of different types, e.g. char * and char ** which might be aliased and they have different types.

510 ↗(On Diff #74219)

Converted to be function scoped.

512 ↗(On Diff #74219)

Converted to be function scoped.

515 ↗(On Diff #74219)

Converted to be function scoped.

515–516 ↗(On Diff #74219)

Removed as unused.

522 ↗(On Diff #74219)

Removed as unused.

524 ↗(On Diff #74219)

This is not safe. Replaced with unique_ptr.

567 ↗(On Diff #74219)

Removed as unused.

eastig retitled this revision from [NFC] Loop Versioning for LICM to [NFC] Loop Versioning for LICM code clean up.Oct 11 2016, 4:24 AM

Any objections to commit these changes?

sbaranga added inline comments.Oct 12 2016, 11:03 AM
305 ↗(On Diff #74219)

I agree, this does look strange (and it might not be a NFC), I think either Ashutosh or Adam should comment on this.

524 ↗(On Diff #74219)

Why is this not safe? It looks like if we don't use use unique_ptr we end up with less memory usage (unique_ptr won't free the memory until LoopVersioningLICMs destructor gets called or until we run on another loop).

eastig added inline comments.Oct 12 2016, 1:59 PM
305 ↗(On Diff #74219)

I'll revert for this review.

524 ↗(On Diff #74219)

All separate new and delete are unsafe. Yes, you are right it's hold till destruction of a LoopVersioningLICM object. I'll fix this.

eastig updated this revision to Diff 74441.Oct 12 2016, 2:41 PM

Updated according to Silviu's comments

Changes looks OK, except one minor formatting comment.

166 ↗(On Diff #74441)

Please correct formatting, can use clang-format here.

305 ↗(On Diff #74219)

Type check is required for LICM, it expects pointers in the alias set should have the same type.

eastig added inline comments.Oct 13 2016, 2:34 AM
305 ↗(On Diff #74219)

Yes, it is required but LICM does it for alias sets with must-alias pointers. Here an alias set with may-alias pointers is checked. The goal of the LoopVersioningLICM pass is to create a version of a loop where all alias sets with may-alias pointers are converted either into sets with must-alias pointers or single-element sets. The checking is similar to what LICM does but it has different meaning. It restricts may-alias pointers used in a loop to have the same type. It excludes a case when 'char *' pointer can be aliased with everything. For example the following loop is not versioned because of this checking:

void f(int *a, int *b, char *c, int n) {
  for (int i = 0; i < n; ++i) {
    a[i] = b[i] + *c;
    *c = (char)a[i];

In this case an alias set consists of 'a', 'b' and 'c'. We can calculate bounds of all pointers used in the loop. So we can create RT checks and move memory accesses to 'c' out of the loop.

eastig updated this revision to Diff 74491.Oct 13 2016, 3:30 AM
eastig edited edge metadata.

Done code formatting.

I assume the changes are approved. I am committing them.

ashutosh.nema accepted this revision.Oct 14 2016, 1:20 AM
ashutosh.nema edited edge metadata.


This revision is now accepted and ready to land.Oct 14 2016, 1:20 AM
sbaranga added inline comments.Oct 14 2016, 2:25 AM
522 ↗(On Diff #74491)

This only releases the memory when running on a new loop. So it wouldn't make things better. The solution also looks a bit over-engineered.

524 ↗(On Diff #74219)

I still don't understand why new/delete would be unsafe.

sbaranga accepted this revision.Oct 14 2016, 2:37 AM
sbaranga edited edge metadata.


522 ↗(On Diff #74491)

Ok, Evgeny just explained to me that this would release the memory when it goes out of scope. I think it's fine now.

This revision was automatically updated to reflect the committed changes.