This is an archive of the discontinued LLVM Phabricator instance.

[PM] Port NaryReassociate to the new PM
ClosedPublic

Authored by wmi on Jul 21 2016, 2:08 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi updated this revision to Diff 64955.Jul 21 2016, 2:08 PM
wmi retitled this revision from to [PM] Port NaryReassociate to the new PM.
wmi updated this object.
wmi added reviewers: davidxl, davide, silvas.
wmi set the repository for this revision to rL LLVM.
wmi added a subscriber: llvm-commits.
wmi updated this revision to Diff 64962.Jul 21 2016, 2:17 PM

Missed to include include/llvm/Transforms/Scalar/NaryReassociate.h into the diff.

davidxl added inline comments.Jul 21 2016, 2:39 PM
test/Transforms/NaryReassociate/nary-mul.ll
2

How about other tests in the same dir?

wmi added inline comments.Jul 21 2016, 2:46 PM
test/Transforms/NaryReassociate/nary-mul.ll
2

I saw the guildeline in https://reviews.llvm.org/rL272505 said one test changed was enough. I just tried other tests and they passed. Do you want me to include them?

wmi updated this revision to Diff 64970.Jul 21 2016, 2:56 PM

Addressed David's comment.

davide accepted this revision.Jul 21 2016, 2:57 PM
davide edited edge metadata.

lgtm but please wait for David

lib/Transforms/Scalar/NaryReassociate.cpp
162

Can you add a TODO for setPreservedCFG as other passes do?

test/Transforms/NaryReassociate/nary-mul.ll
2

I agree that porting a single test is (generally) enough, unless there's a reason to port more for now (e.g. to stress interaction between transform passes and analyses).

This revision is now accepted and ready to land.Jul 21 2016, 2:57 PM
davidxl accepted this revision.Jul 21 2016, 3:07 PM
davidxl edited edge metadata.

lgtm

wmi updated this revision to Diff 64973.Jul 21 2016, 3:11 PM
wmi edited edge metadata.

Add two more tests under test/Transforms/NaryReassociate/NVPTX/.
Address Davide's comment.

This revision was automatically updated to reflect the committed changes.