Page MenuHomePhabricator

[GVN][NFC] Refactor code and add description for GVN object
Needs ReviewPublic

Authored by ksyx on Apr 9 2021, 4:44 AM.

Details

Summary

In this revision:

  • Added descriptions for the GVN object.
    • The original comment says a "good summary of algorithm implemented by this GVN object" is required, but I am not sure how deep to get into this description. Thus, the description I added focused on the work done and relationships among methods.
  • Moved the code assigning new value number into a new method to improve code tidiness.
  • Since a if statement before a line deleted guaranteed that StoreOffset + StoreSize >= LoadOffset + LoadSize, it is guaranteed that StoreOffset + StoreSize > LoadOffset so the being removed in this revision might be redundant.
  • Fix typos and spaces.

Diff Detail

Event Timeline

ksyx created this revision.Apr 9 2021, 4:44 AM
ksyx requested review of this revision.Apr 9 2021, 4:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2021, 4:44 AM
ksyx updated this revision to Diff 336414.Apr 9 2021, 5:35 AM

fix: unable to apply patch

ksyx updated this revision to Diff 336422.Apr 9 2021, 6:01 AM

fix: clang-format failed

lattner resigned from this revision.Apr 9 2021, 3:15 PM

The patch looks reasonable to me, but I'm no longer a competent reviewer for this area.

ksyx added a reviewer: fhahn.Apr 9 2021, 4:00 PM
nikic added a comment.Apr 11 2021, 7:57 AM

Just FYI, recently work on NewGVN has started again, so that's where the focus will (hopefully) be in the future.

llvm/include/llvm/Transforms/Scalar/GVN.h
112

I think this comment would do better in the cpp file, closer to the implementation.

116

I don't think this is needed, this is just general pass setup.

122

reversed order -> reverse post-order (or just RPO)

131

was -> is

llvm/lib/Transforms/Utils/VNCoercion.cpp
217

Looks right to me, feel free to just commit this separately.

ksyx marked an inline comment as done.EditedApr 11 2021, 5:14 PM

Just FYI, recently work on NewGVN has started again, so that's where the focus will (hopefully) be in the future.

That’s fine. By the way, will it be changed into something like LegacyGVN or just remove the whole?

ksyx updated this revision to Diff 336714.Apr 11 2021, 6:58 PM
ksyx marked 4 inline comments as done.

apply suggestions from code review.

ksyx added a comment.Wed, May 5, 5:33 AM

Ping :)
Please tell me if there is anything that could be improved, thanks!