This is an archive of the discontinued LLVM Phabricator instance.

[CodeExtractor] Fix multiple bugs under certain shape of extracted region
ClosedPublic

Authored by myhsu on Sep 15 2017, 7:06 AM.

Details

Summary

If the extracted region has multiple exported data flows toward the same BB which is not included in the region, correct resotre instructions and PHI nodes won't be generated inside the exitStub. The solution is simply put the restore instructions right after the definition of output values instead of putting in exitStub.
Unittest for this bug is included

Diff Detail

Event Timeline

myhsu created this revision.Sep 15 2017, 7:06 AM
davide edited edge metadata.
davide added subscribers: kuhar, dberlin.

I'm a little nervous about updating the dominator manually, still. I wonder if we can use the new API that Jakub implemented (cc: @kuhar / @dberlin )

Please don't update it manually.
Pretty much every manual update LLVM does has been proven incorrect over time.
Please just express the added/removed edges if you have to.
If you are cutting out a region, this should not be difficult.

kuhar requested changes to this revision.EditedSep 15 2017, 10:04 AM

I have to agree with Danny, I'm not sure this version is correct. It should be fairly easy to use the batch updater here.
If we discover that doing incremental update makes the code too slow, that it's also great, as I still haven't come up a good benchmark to tell me what needs more optimization.

Nit: please run clang-format on your changes.

lib/Transforms/Utils/CodeExtractor.cpp
473

Does this handle the self-loop case?

480

In general, we prefer to update DFS numbers lazily and it's better to just invalidate them.

This revision now requires changes to proceed.Sep 15 2017, 10:04 AM
kuhar added inline comments.Sep 15 2017, 10:23 AM
lib/Transforms/Utils/CodeExtractor.cpp
771

You can use auto here, as the type is obvious looking at the right hand side.

myhsu updated this revision to Diff 115649.Sep 18 2017, 7:46 AM
myhsu edited edge metadata.
myhsu edited the summary of this revision. (Show Details)
myhsu added a comment.Sep 18 2017, 7:51 AM

It turn out that the DomTree updating issue has actually been fixed in D32308
The unittest also pass after I applying that change
Sorry I didn't work incrementally on master branch at first place

myhsu marked 2 inline comments as done.Sep 21 2017, 12:48 PM

@kuhar @dberlin
Could you help reviewing my update?
Thanks : )

chandlerc edited edge metadata.Sep 21 2017, 4:06 PM

Can you revert the unrelated formatting changes? Otherwise reviewing this is a bit hard.

myhsu updated this revision to Diff 116334.Sep 22 2017, 5:20 AM

@chandlerc
Temporarily revert to version before applying clang-format

myhsu edited the summary of this revision. (Show Details)Sep 22 2017, 5:23 AM
kuhar added a comment.Oct 2 2017, 6:30 AM

@chandlerc
Temporarily revert to version before applying clang-format

I think the point was to only format your changes, not the surrounding code. You can use the clang-format-diff.py script to to that.

lib/Transforms/Utils/CodeExtractor.cpp
791

Why is it the case that incrementing OAI works here and gets to OutputArgEnd? Same with the GEP::Create, why is passing the OAI pointer there valid if it doesn't get incremented and you use outputs[i] the same time?

This is not clear to me, could you consider adding a comment?
I think that the comment from the previous version of this snippet got lost here.

unittests/Transforms/Utils/CodeExtractor.cpp
57

nit: s/require/requires
Maybe it'd be better to phrase is like CodeExtractor requires the first element to dominate all the other ones?. Isn't an element really a BasicBlock here?

68

Why is FALSE expected here?

myhsu updated this revision to Diff 117383.Oct 2 2017, 10:25 AM
myhsu marked 3 inline comments as done.

@kuhar
Fix things mentioned in the inline comments

myhsu added inline comments.Oct 2 2017, 10:29 AM
lib/Transforms/Utils/CodeExtractor.cpp
791

The original code didn't check the OutputArgEnd either, however I add an assertion anyway.

OAI iterator shouldn't be increased in each iteration if AggregateArgs is true. Since under the configuration of using aggregate argument, there would be only one struct argument that accommodate all of the output values in struct fields, and OAI should always point to that struct argument.

The code comment in the original code seems to be not related to the aggregate argument condition, but I add few comment regarding this concern anyway

unittests/Transforms/Utils/CodeExtractor.cpp
68

verifyFunction would return false if no error

myhsu updated this revision to Diff 117387.Oct 2 2017, 10:34 AM

Fix typo

myhsu marked an inline comment as done.Oct 2 2017, 10:34 AM
kuhar added inline comments.Oct 2 2017, 10:48 AM
lib/Transforms/Utils/CodeExtractor.cpp
745–746

nit: s/reference/reference.
Comments should be full sentences and end with ..

770

nit: s/value/value.

782

nit: maybe s/amount/number?

792

Maybe s/increase/increment or even s/increase/advance?

793

nit: s/point/points
s/case/case.

unittests/Transforms/Utils/CodeExtractor.cpp
59

nit: s/ones/ones.

68

Maybe that's obvious for people more familiar with this part of the codebase, but I'd appreciate a comment explaining that here :)

myhsu marked 6 inline comments as done.Oct 3 2017, 2:14 PM
kuhar accepted this revision.Oct 3 2017, 2:36 PM

Other than the two minor nits, the patch looks good to me.

lib/Transforms/Utils/CodeExtractor.cpp
774

nit: s/point/point.

777

nit: s/PHIs/PHIs.
You can also move this piece of comment to the line above, as you won't hit the 80-column limit (I think :P).

This revision is now accepted and ready to land.Oct 3 2017, 2:36 PM
myhsu updated this revision to Diff 117658.Oct 4 2017, 5:14 AM

Fix comment

myhsu marked 2 inline comments as done.Oct 4 2017, 5:16 AM

Other than the two minor nits, the patch looks good to me.

Thanks @kuhar :)

myhsu added a comment.Oct 5 2017, 5:22 AM

@chandlerc @kuhar
I'm not pretty familiar the procedure yet, could someone help me commit the patch or merge it?
Thanks

kuhar added a comment.Oct 5 2017, 7:15 AM

@chandlerc @kuhar
I'm not pretty familiar the procedure yet, could someone help me commit the patch or merge it?
Thanks

If you have commit access, you can go ahead and submit the patch. Otherwise, I can submit it for you.
Please take a look here: Obtaining Commit Access.

myhsu added a comment.Oct 5 2017, 7:58 AM

@chandlerc @kuhar
I'm not pretty familiar the procedure yet, could someone help me commit the patch or merge it?
Thanks

If you have commit access, you can go ahead and submit the patch. Otherwise, I can submit it for you.
Please take a look here: Obtaining Commit Access.

Since this is my first time committing a patch, I guess I don't have enough record for being granted the commit access
Please submit this changed for me, thanks!

kuhar added a comment.Oct 5 2017, 8:10 AM

Can you please rebase the patch so that is applies cleanly to the current ToT?

myhsu updated this revision to Diff 117828.Oct 5 2017, 8:28 AM

Rebase to master

kuhar added a comment.Oct 5 2017, 1:19 PM

With your patch, FindPhiPredForUseInBlock becomes unused and it causes the following warning to be emitted:

llvm/lib/Transforms/Utils/CodeExtractor.cpp:657:20: warning: unused function 'FindPhiPredForUseInBlock' [-Wunused-function]
static BasicBlock* FindPhiPredForUseInBlock(Value* Used, BasicBlock* BB) {

And that would make many buildbots very grumpy :)
I think that the function should be now deleted, as it has no users.

kuhar requested changes to this revision.Oct 5 2017, 1:46 PM
This revision now requires changes to proceed.Oct 5 2017, 1:46 PM
myhsu updated this revision to Diff 117944.Oct 5 2017, 6:16 PM
myhsu edited edge metadata.

Remove 'FindPhiPredForUsedInBlock' function

This revision was automatically updated to reflect the committed changes.