This is an archive of the discontinued LLVM Phabricator instance.

[PM] LoopAccessInfo Refactor #2
ClosedPublic

Authored by davidxl on Jun 22 2016, 11:58 PM.

Details

Summary

In this patch, move constructor/assignment operator is defined for LoopAccessInfo. PredicatedScalarEvolution PSE member is also changed to be unique_ptr.

Diff Detail

Repository
rL LLVM

Event Timeline

davidxl updated this revision to Diff 61644.Jun 22 2016, 11:58 PM
davidxl retitled this revision from to LoopAccessInfo Refactor-2.
davidxl updated this object.
davidxl retitled this revision from LoopAccessInfo Refactor-2 to [PM] LoopAccessInfo Refactor #2.Jun 23 2016, 12:02 AM
davidxl updated this object.
davidxl added reviewers: anemet, davide, silvas.
davidxl edited subscribers, added: llvm-commits; removed: sanjoy, mzolotukhin.

Minor drop by comment inline.

include/llvm/Analysis/LoopAccessAnalysis.h
530 ↗(On Diff #61644)

This looks strange -- when will this check be true?

majnemer added inline comments.
include/llvm/Analysis/LoopAccessAnalysis.h
530 ↗(On Diff #61644)

Self assignment.

sanjoy added inline comments.Jun 23 2016, 12:50 AM
include/llvm/Analysis/LoopAccessAnalysis.h
530 ↗(On Diff #61644)

I mean -- do we need to support

x = std::move(x);

? Or is it better to assert out in such cases? Are there ways such an assignment can come up organically?

davidxl added inline comments.Jun 23 2016, 9:13 AM
include/llvm/Analysis/LoopAccessAnalysis.h
530 ↗(On Diff #61644)

not likely. I can change it to assert as you suggested.

davidxl updated this revision to Diff 61807.Jun 24 2016, 10:42 AM

Addressed review feedback.

anemet added inline comments.Jun 30 2016, 10:05 AM
include/llvm/Analysis/LoopAccessAnalysis.h
518–527 ↗(On Diff #61807)

How are these different from the default move ctor/assignment operator?

633 ↗(On Diff #61807)

Hmm, const member returning a non-const reference?

634 ↗(On Diff #61807)

Can we make this private now?

770–780 ↗(On Diff #61807)

Does this belong to this patch?

davidxl marked 2 inline comments as done.Jun 30 2016, 11:38 AM
davidxl added inline comments.
include/llvm/Analysis/LoopAccessAnalysis.h
518–527 ↗(On Diff #61807)

Sean mentioned that some compiler (MVS) does not generate default move ctor properly.

anemet added inline comments.Jun 30 2016, 11:40 AM
include/llvm/Analysis/LoopAccessAnalysis.h
518–527 ↗(On Diff #61807)

So they're not different?

A comment explaining this would be good.

davidxl updated this revision to Diff 62389.Jun 30 2016, 11:43 AM

Addressed Adams' feedbacks.

davidxl updated this revision to Diff 62397.Jun 30 2016, 12:11 PM

Forcing the generation of default move ctor.

silvas edited edge metadata.Jun 30 2016, 4:14 PM

Quick comment to avoid a buildbot issue.

include/llvm/Analysis/LoopAccessAnalysis.h
518 ↗(On Diff #62397)

The issue with MSVC is actually that it can't synthesize this, so = default doesn't help unfortunately. You have to manually write out the entire move ctor like you had it. Just make sure to add a comment " // Hack for MSVC 2013 which seems like it can't synthesize this." or similar.

davidxl updated this revision to Diff 62440.Jun 30 2016, 4:35 PM
davidxl edited edge metadata.

Explicitly define move ctor to avoid potential bot failure.

anemet accepted this revision.Jun 30 2016, 4:37 PM
anemet edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jun 30 2016, 4:37 PM
This revision was automatically updated to reflect the committed changes.
davide edited edge metadata.Jul 1 2016, 5:08 PM

Sorry David, I completely missed this. LGTM, anyway.