This is an archive of the discontinued LLVM Phabricator instance.

[GVN] AnalyzeLoadAvailability: Replace a load after lifetime.end with undef (PR20811)
Needs RevisionPublic

Authored by BK1603 on Aug 22 2019, 10:31 AM.

Details

Summary

This commit attempts on fixing bug #20811 [ https://bugs.llvm.org/show_bug.cgi?id=20811 ]

The commit changes the function AnalyzeLoadAvailability in the following manner:

Currently, the function considers a load after lifetime end to be a instruction clobbered dependency,
and returns false, without populating Res.

The commit makes the function return true if a load is performed after the lifetime of a pointer has ended
and populates Res with undef (just as in the case of a load placed immediately after lifetime start.)

Event Timeline

BK1603 created this revision.Aug 22 2019, 10:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2019, 10:31 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
BK1603 retitled this revision from # Enter a commit message. # # Changes: # # llvm/lib/Transforms/Scalar/GVN.cpp to Fix for bug #20811.
BK1603 updated this revision to Diff 216667.EditedAug 22 2019, 10:44 AM

Added test

BK1603 retitled this revision from Fix for bug #20811 to [GVN] AnalyzeLoadAvailability: Replace a load after lifetime.end with undef (PR20811).Aug 30 2019, 6:42 AM
BK1603 edited the summary of this revision. (Show Details)
fhahn added a comment.Sep 1 2019, 9:03 AM

Thanks for the patch and sorry for the long response time

llvm/lib/Transforms/Scalar/GVN.cpp
874

I suppose the reason for moving this was that we do not get a Def dependency for lifetime_end?

I think we should keep the original checks at their original positions, we should only get those for defs. The handling of lifetime_end can be moved in the block dealing with clobbers.

BK1603 updated this revision to Diff 218267.Sep 1 2019, 9:43 AM

Replaced conditionals at original position.

BK1603 added a comment.Sep 1 2019, 9:48 AM

@fhahn Sure! Also no worries :)
Thanks for the review.

I've made the changes, can you verify them?

xbolva00 added inline comments.Sep 1 2019, 9:52 AM
llvm/lib/Transforms/Scalar/GVN.cpp
876

if (

@xbolva00 Couldn't understand what you meant by that comment?
Also after making the changes, should I mark them done, or does the person
who made the comment verifies it and then marks them done?

“if(“ is not LLVM-style. Please run clang format on your code changes.

should I mark them done

Yes, if you fixed review comments, please mark them as done.

BK1603 updated this revision to Diff 218268.Sep 1 2019, 10:31 AM
BK1603 marked an inline comment as done.

Refactored code.

fhahn added a comment.Sep 12 2019, 1:32 PM

Thanks! Could you also add another test where we have a call to lifetime.end, followed by another defining store, followed by a load? This should not get optimized by this patch.

llvm/lib/Transforms/Scalar/GVN.cpp
873

please remove the unrelated whitespace change.

Does NewGVN optimize this already? it will be good to have it there as well.

BK1603 updated this revision to Diff 220240.Sep 14 2019, 10:19 PM
BK1603 marked 2 inline comments as done.

Added test case when lifetime end is followed by a defining store. A load in this case should not be
optimized by GVN.

BK1603 updated this revision to Diff 220241.Sep 14 2019, 10:23 PM

Removed whitespace.

BK1603 marked an inline comment as done.Sep 14 2019, 10:25 PM

Thanks! Could you also add another test where we have a call to lifetime.end, followed by another defining store, followed by a load? This should not get optimized by this patch.

I have added a test case for it. :)
Also are the name for the test files fine? Or should I change them?

Does NewGVN optimize this already? it will be good to have it there as well.

Sure! :)

Though I think I should wait for this patch to be submitted first, once it is, I can make similar changes to NewGVN.
I hope that is fine?

xbolva00 added inline comments.Sep 15 2019, 9:16 AM
llvm/test/Transforms/GVN/lifetime-end-store.ll
3

You can move this test to lifetime-end.ll as “test2”.

llvm/test/Transforms/GVN/lifetime-end.ll
1

You can use llvm/utils/update_test_checks.py to generate CHECK lines for you - much better solution than hand-written check lines.

BK1603 updated this revision to Diff 222099.Sep 27 2019, 1:55 AM

Moved test to the same file

BK1603 marked an inline comment as done.Sep 27 2019, 1:56 AM

Sorry for the late response ':)

I moved the test to the same file as test2, and well, copy pasted the CHECK lines and the RUN lines for now, I hope its fine?

Looks good!

llvm/test/Transforms/GVN/lifetime-end.ll
14

Remove this run line.

BK1603 updated this revision to Diff 222136.Sep 27 2019, 5:14 AM

Removed unnecessary RUN line

xbolva00 accepted this revision.Sep 27 2019, 6:25 AM
This revision is now accepted and ready to land.Sep 27 2019, 6:25 AM

@fhahn , is it fine for you?

@BK1603, should somebody land this patch for you?

BK1603 marked an inline comment as done.Oct 10 2019, 8:57 PM

@xbolva00
Yes, i dont have commit access, i need someone to land this patch for me :)

fhahn added a comment.Oct 11 2019, 1:46 AM

I can commit it for you in a bit, thanks for the patch! I'll generate the check lines before committing.

fhahn requested changes to this revision.Oct 11 2019, 4:07 AM

This seems to cause MultiSource/Benchmarks/DOE-ProxyApps-C++/HPCCG/HPCCG and External/SPEC/CINT2000/176.gcc/176.gcc from https://github.com/llvm/test-suite to fail.

This revision now requires changes to proceed.Oct 11 2019, 4:07 AM
fhahn added a comment.Oct 11 2019, 4:08 AM

This seems to cause MultiSource/Benchmarks/DOE-ProxyApps-C++/HPCCG/HPCCG and External/SPEC/CINT2000/176.gcc/176.gcc from https://github.com/llvm/test-suite to fail.

It's not an actual runtime failure, both won't terminate.

BK1603 added a comment.EditedOct 15 2019, 5:08 AM

@fhahn

Sorry for the recent slow down in work on this patch ':)
I will check it out tonight and try and fix it!

xbolva00 resigned from this revision.Nov 15 2019, 4:41 PM