This is an archive of the discontinued LLVM Phabricator instance.

Simplify main statement flag in ScopBuilder::buildEqivClassBlockStmts
ClosedPublic

Authored by bin.narwal on Aug 20 2019, 7:55 AM.

Details

Summary

Hi,
When reading code in ScopBuilder::buildEqivClassBlockStmts, I think the main statement flag computation can be simplified, here is the patch. It's based on two simple facts that: 1, Instruction won't be removed once it's inserted into UnionFind. 2, Main statement must be set if there is non-trivial statement besides the last one. The patch also saves std::find call. Any comments?

BTW, this is my first patch to LLVM/Polly. Please correct me if there is anything wrong. Thanks in advance.

Thanks,

Diff Detail

Repository
rL LLVM

Event Timeline

bin.narwal created this revision.Aug 20 2019, 7:55 AM
Herald added a project: Restricted Project. · View Herald Transcript

The change looks good. Thank you for the contribution! Could you change the comments a bit as remarked inline?

Once you updated this diff, I can do the commit with a "Patch by: bin.narwal <bin.narwal@gmail.com>" line if you want.

lib/Analysis/ScopBuilder.cpp
2144 ↗(On Diff #216147)

"yet" does not fit the context: The decision of which is the main statement has been made before, and will not change during this loop. I suggest to just remove the "yet" (or keep the original comment, I don't see any change in meaning)

2157 ↗(On Diff #216147)

Could you add a comment about that Count == 0 means that there was no other main? Suggestion:

// The epilogue becomes the main statement only if there is no other statement that could become main.

Hi Meinersbur,
Thanks very much for reviewing. Here is the updated patch according to your comments.

Thanks,

This revision was not accepted when it landed; it landed in state Needs Review.Aug 26 2019, 2:43 PM
This revision was automatically updated to reflect the committed changes.