This is an archive of the discontinued LLVM Phabricator instance.

[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
111

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

117

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

123

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

132

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.May 5 2021, 5:33 AM

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

The bit that was split off and committed as rGc35e8185d8c170c20e28956e0c9f3c1be895fefb was not an NFC, it broke compilation of one file for me: https://martin.st/temp/vlc-preproc.c

$ clang -target armv7-w64-mingw32 -w -c -O2 vlc-preproc.c 
clang: ../lib/Transforms/Utils/VNCoercion.cpp:475: llvm::Value* llvm::VNCoercion::getLoadValueForLoad(llvm::LoadInst*, unsigned int, llvm::Type*, llvm::Instruction*, const llvm::DataLayout&): Assertion `SrcVal-isSimple() && "Cannot widen volatile/atomic load!"' failed.

I'd suggest reverting that patch until this issue is sorted out.

ksyx added a comment.Nov 15 2021, 4:37 AM

The bit that was split off and committed as rGc35e8185d8c170c20e28956e0c9f3c1be895fefb was not an NFC, it broke compilation of one file for me: https://martin.st/temp/vlc-preproc.c

$ clang -target armv7-w64-mingw32 -w -c -O2 vlc-preproc.c 
clang: ../lib/Transforms/Utils/VNCoercion.cpp:475: llvm::Value* llvm::VNCoercion::getLoadValueForLoad(llvm::LoadInst*, unsigned int, llvm::Type*, llvm::Instruction*, const llvm::DataLayout&): Assertion `SrcVal-isSimple() && "Cannot widen volatile/atomic load!"' failed.

I'd suggest reverting that patch until this issue is sorted out.

Thanks for your feedback! I am going to figure out that when I had time and I will revert it soon.

ksyx added a comment.EditedNov 15 2021, 6:16 AM

@mstorsjo

Reproduced. The commit is reverted now. Please check if the compilation goes back to normal. Sorry for inconvenience.

Reversion: 72b5138d37d7ec799301e29e9c1eaaa1630be161

@mstorsjo

Reproduced. The commit is reverted now. Please check if the compilation goes back to normal. Sorry for inconvenience.

Reversion: 72b5138d37d7ec799301e29e9c1eaaa1630be161

Thanks, now it does seem to build correctly again.

ksyx added a comment.EditedNov 19 2021, 5:28 PM

@mstorsjo

I have made fix to my patch and your problem seemly to be solved in my local test. Please check again and thanks for feedback again.

[build]$ bin/clang -target armv7-w64-mingw32 -w -c -O2 vlc-preproc.c
[build]$ echo $?
0

I think I may add some checks sometime.

@mstorsjo

I have made fix to my patch and your problem seemly to be solved in my local test. Please check again and thanks for feedback again.

[build]$ bin/clang -target armv7-w64-mingw32 -w -c -O2 vlc-preproc.c
[build]$ echo $?
0

I think I may add some checks sometime.

Thanks, the original case seems to still build fine now.