Page MenuHomePhabricator

WIP: Prototype of DSE optimizations for -ftrivial-auto-var-init
Needs ReviewPublic

Authored by vitalybuka on May 13 2019, 7:28 PM.

Details

Reviewers
jfb
Summary

I don't want to invest into improving this patch. I will extract useful parts
and send them for review using this one as a benchmark reference.

Coroutines tests do not like pass structure so I disable the to figure out
later.

If you try this patch and find cases which module level DSE should handle,
and this one does not, especially -ftrivial-auto-var-init=patter related, please
share them with me.

Event Timeline

vitalybuka created this revision.May 13 2019, 7:28 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 13 2019, 7:28 PM
vitalybuka planned changes to this revision.May 13 2019, 7:29 PM
jfb added a reviewer: jfb.Jun 20 2019, 9:04 AM

Hello! I'm still interested in this, have you been able to make any progress?

glider added a subscriber: glider.Jul 3 2019, 3:02 AM

Vitaly, can you please rebase the patch?
As far as I can see, you've submitted parts of it already.
(not that I can't resolve the conflicts locally, but keeping it up-to-date may save others some time)

glider added inline comments.Jul 3 2019, 3:11 AM
llvm/lib/Transforms/Scalar/DeadStoreEliminationExpGlobal.cpp
20

error: zero-size array ‘llvm::GUIDS_LOOKUP’

Can you please provide an example of DeadStoreEliminationExpGlobalGUIDListGen.h? The instruction seems insufficient.

27

Ditto.

glider added inline comments.Jul 5 2019, 6:25 AM
llvm/lib/Transforms/Scalar/DeadStoreEliminationExp.cpp
709

This cast fails in the following case:

%21 = call i64 @probe_kernel_read(i8* nonnull %9, i8* %20, i64 8) #10, !dbg !7740

, where the callee is declared as:

@probe_kernel_read = weak hidden alias i64 (i8*, i8*, i64), i64 (i8*, i8*, i64)* @__probe_kernel_read

When building Android kernel with LTO enabled, the gold plugin crashes on an assertion.

glider added inline comments.Jul 5 2019, 10:11 AM
llvm/lib/Transforms/Scalar/DeadStoreEliminationExp.cpp
556

An assertion fires here on the Android kernel:

aarch64-linux-android-ld.gold: /usr/local/google/src/llvm-git-monorepo/llvm/lib/IR/ConstantRange.cpp:54: llvm::ConstantRange::ConstantRange(llvm::APInt, llvm::APInt): Assertion `(Lower != Upper || (Lower.isMaxValue() || Lower.isMinValue())) && "Lower == Upper, but they aren't min or max value!"' failed.

For some reason the (APInt, APInt) version of the constructor is being invoked.
Probably PointerSizeInBits should be declared as int32_t here and in findDeadStores()

563

I'm seeing a case in which Range.getLower() is 20179 and LS.getValue() is 0.

fhahn added a subscriber: fhahn.Jul 5 2019, 11:09 AM
asbirlea added inline comments.Jul 8 2019, 12:11 PM
llvm/include/llvm/Analysis/MemorySSA.h
1109

Could changes in this file be landed separately?
They seem unrelated and not in need of review.

vitalybuka marked 2 inline comments as done.Jul 8 2019, 12:47 PM
vitalybuka added inline comments.
llvm/include/llvm/Analysis/MemorySSA.h
1109

It's already there r364247, i didn't rebase the patch after submit.

llvm/lib/Transforms/Scalar/DeadStoreEliminationExpGlobal.cpp
27

These files should be empty. Raw diff https://reviews.llvm.org/file/data/o6sk6gw2gqs4u4pmodrn/PHID-FILE-s6c6nsofxnqekkcvzdzs/D61879.diff already contains them.
It's ThinLTO replacement experiments. They don't improve results enough, so I didn't bother to create real ThinLTO stuff. Anyway it is not needed for full LTO.

Rebase only

asbirlea removed a subscriber: asbirlea.Jul 15 2019, 2:20 PM
glider added inline comments.Jul 16 2019, 6:55 AM
llvm/lib/Transforms/Scalar/DeadStoreEliminationExpGlobal.cpp
27
$ ninja -j64 check-clang
...
/usr/local/google/src/llvm-git-monorepo/llvm/lib/Transforms/Scalar/DeadStoreEliminationExpGlobal.cpp:20:32: error: zero-size array ‘llvm::GUIDS_LOOKUP’
 static const GlobalValue::GUID GUIDS_LOOKUP[] = {
                                ^~~~~~~~~~~~
/usr/local/google/src/llvm-git-monorepo/llvm/lib/Transforms/Scalar/DeadStoreEliminationExpGlobal.cpp:26:19: error: zero-size array ‘llvm::GlobalArgumentMemAccess’
 static const char GlobalArgumentMemAccess[] = {
                   ^~~~~~~~~~~~~~~~~~~~~~~

Am I doing something wrong? Looks like empty files aren't enough.
I've fixed the problem by putting "0" into each file, but it's strange it works differently for us.

vitalybuka marked an inline comment as done.Jul 16 2019, 1:53 PM
vitalybuka added inline comments.
llvm/lib/Transforms/Scalar/DeadStoreEliminationExpGlobal.cpp
27

You must be using LLVM_ENABLE_WERROR?

fix LTO on kernel

Vitaly, what's the current status of these patches? What's your plan on submitting them?

Please consider adding a test.