This is an archive of the discontinued LLVM Phabricator instance.

[LoopDist/LoopVer] Move LoopVersioning to a new module, NFC
ClosedPublic

Authored by anemet on Jun 19 2015, 2:58 PM.

Details

Summary

This is a verbatim move except for expanded comment before the class,
the new assert in the constructor and some clang-format fixes due to the
changed indentation level in the function defs.

The class will obviously need improvement down the road. For one, there
is no reason that addPHINodes would have to be exposed like that. I
will make this and other improvements in follow-up patches.

The main goal is to be able to share this functionality. The
LoopLoadElimination pass I am working on needs it too. Later we can
move other clients as well (LV and Ashutosh's LICMVer).

Diff Detail

Repository
rL LLVM

Event Timeline

anemet updated this revision to Diff 28052.Jun 19 2015, 2:58 PM
anemet retitled this revision from to [LoopDist/LoopVer] Move LoopVersioning to a new module, NFC.
anemet updated this object.
anemet edited the test plan for this revision. (Show Details)
anemet added reviewers: hfinkel, ashutosh.nema.
anemet added a subscriber: Unknown Object (MLST).
anemet updated this revision to Diff 28170.Jun 22 2015, 4:24 PM

Rebase after r240352 ([LoopDist] Improve variable names and comments in LoopVersioning class, NFC)

anemet updated this revision to Diff 28171.Jun 22 2015, 4:30 PM

OK, let's try this again without merging the two pending patches...

hfinkel added inline comments.Jun 22 2015, 6:34 PM
include/llvm/Transforms/Utils/LoopVersioning.h
10 ↗(On Diff #28171)

This is a stale comment?

44 ↗(On Diff #28171)

From this interface, I find it very unclear how you get out the new versioned loop after the CFG mutation is complete.

lib/Transforms/Utils/LoopVersioning.cpp
10 ↗(On Diff #28171)

Stale comment?

anemet added inline comments.Jun 22 2015, 9:32 PM
include/llvm/Transforms/Utils/LoopVersioning.h
10 ↗(On Diff #28171)

Yes, sorry.

44 ↗(On Diff #28171)

The loop that was used to construct the class will be the "versioned" loop i.e. the loop that will get control if all the memchecks pass.

This is the loop that typically, the client transform will be interested in modifying. E.g. this is what LoopDistribute does.

This is sort of implicit because the versioning is optional. I think this is a typical algorithm:

for each loop L:
  analyze L
  if versioning is necessary version L
  transform L

The versioning itself is an optional step and you don't change the loop underlying the transformation if versioning was necessary.

Both loops (versioned, unversioned) are attributes of the class, so we can add APIs to query them as we need them.

Let me know if you'd be OK just commenting the above or you're requesting an actual API change.

Thanks very much for the review!

Adam

hfinkel added inline comments.Jun 22 2015, 10:33 PM
include/llvm/Transforms/Utils/LoopVersioning.h
44 ↗(On Diff #28171)

In that case, I think the current API is okay, but needs comments.

49 ↗(On Diff #28171)

This function too needs comments on when/how it should be used.

anemet added inline comments.Jun 22 2015, 11:00 PM
include/llvm/Transforms/Utils/LoopVersioning.h
44 ↗(On Diff #28171)

OK, will add.

49 ↗(On Diff #28171)

Sure, I'll explain it.

Just FYI, as I was mentioning in the summary, I'd like to make this a private member function in a follow-on patch, so clients would not have to worry about when/where to call it. This can be called at the end of versionLoop, internally.

Where I'd like to end up is something like this:

  • move findDefsUsedOutside to LoopUtils.h or something like that
  • have LoopVersioning take an optional DefsUsedOutside vector in the ctor. Optional, because like in the case of LoopDistribution the pass itself needs this piece of information so it's readily available
  • compute DefsUsedOutside using findDefsUsedOutside if it wasn't passed
  • call addPHINodes if there are DefsUsedOutside
ashutosh.nema edited edge metadata.Jun 23 2015, 9:55 AM

Thanks Adam for working on this, it looks useful.

General comment:
Please add more comments to functions.

lib/Transforms/Utils/LoopVersioning.cpp
41 ↗(On Diff #28171)

Loop pre header may be null, probably you need to check that.
If you are assuming a preheader then you can put a assert here.

60 ↗(On Diff #28171)

This function does not care about exit blocks, you don't want to clone exit blocks ?

75 ↗(On Diff #28171)

Add some comments what this function does

anemet added inline comments.Jun 23 2015, 11:09 AM
lib/Transforms/Utils/LoopVersioning.cpp
41 ↗(On Diff #28171)

Sounds good. I'll put it in the constructor next to the other assert.

60 ↗(On Diff #28171)

It does not, hence the name ;). Here we may want to create exist blocks and then clone them to preserve SimplifyLoop.

cloneLoopWithPreheader is used in the LoopDistribution as well and in LoopDistribution there is no point of cloning the exit blocks because the preheader of the next distributed loop becomes the current loop's exit block (satisfying SimplifyLoop).

75 ↗(On Diff #28171)

There is some before the decl but Hal also requested more comments there, so more to follow.

Thanks for your review.

Adam

anemet updated this revision to Diff 28309.Jun 23 2015, 5:14 PM
anemet edited edge metadata.

Address review comments

memcheck no-alias resulting branch is original loop(VersionedLoop).
& memcheck alias resulting branch is newly created loop(NonVersionedLoop).

Any specific reason you kept memcheck branch target like this ?

Why can't memcheck no-alias resulting branch targets to NonVersionedLoop
and alias resulting branch targets to VersionedLoop ?

In current implementation if loopVersioningLICM attempts to set no-alias attribute to the VersionedLoop
that is actually it will set no-alias attribute to the original loop.

I'm doubtful here because the variables of the original loop might be used out of loop in that function.
after setting no-alias it might change the intent of program.

My understanding might be wrong here, please clarify.

include/llvm/Transforms/Utils/LoopVersioning.h
67 ↗(On Diff #28309)

Its more clear if we expand meaning of memcheck pass & fail.
i.e. memory/pointers [non]aliased in loop.

71 ↗(On Diff #28309)

Can we expose NonVersionedLoop & VersionedLoop, or provide getter functions for these.
Loop versioning need these to set meta data.

Typo:

  • Loop versioning need these to set meta data.

Its
+ Loop versioning LICM need these to set meta data.

anemet marked 2 inline comments as done.Jul 2 2015, 10:50 AM

Hi Ashutosh,

memcheck no-alias resulting branch is original loop(VersionedLoop).
& memcheck alias resulting branch is newly created loop(NonVersionedLoop).

Any specific reason you kept memcheck branch target like this ?

Why can't memcheck no-alias resulting branch targets to NonVersionedLoop
and alias resulting branch targets to VersionedLoop ?

In current implementation if loopVersioningLICM attempts to set no-alias attribute to the VersionedLoop
that is actually it will set no-alias attribute to the original loop.

I'm doubtful here because the variables of the original loop might be used out of loop in that function.
after setting no-alias it might change the intent of program.

My understanding might be wrong here, please clarify.

The loops flow into a join where the values used outside are merge with phis. So outside users will see the value from whichever way we arrived. If we had an alias the value will come from the non-versioned loop and vice-versa.

Let me know if this helps.

Adam

anemet updated this revision to Diff 28965.Jul 2 2015, 10:54 AM

Address Ashutosh's latest comments

Address Ashutosh's latest comments

Ashutosh,

Does this and the other patch look OK to you now?

Thanks,
Adam

Thanks Adam, This looks OK to me.

You can check with Hal, if he is OK you can proceed.

Regards,
Ashutosh

hfinkel accepted this revision.Jul 8 2015, 5:55 PM
hfinkel edited edge metadata.

Thanks Adam, This looks OK to me.

You can check with Hal, if he is OK you can proceed.

Yes, LGTM.

Regards,
Ashutosh

include/llvm/Transforms/Utils/LoopVersioning.h
67 ↗(On Diff #28965)

got constructed with -> passed to the constructor

This revision is now accepted and ready to land.Jul 8 2015, 5:55 PM
This revision was automatically updated to reflect the committed changes.