[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

uabelho added a comment. I wrote https://bugs.llvm.org/show_bug.cgi?id=49859 about a miscompile that started occuring when turning on memoryssa again in DSE. I suspect that licm doesn't keep memoryssa updated properly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2379892 , @thakis wrote: > Very belated update: We've been running with this on our clang/tot bots for a > few weeks now and turned this on (or, more accurately stopped turning it off) > for "normal" builds today. All tes

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

thakis added a comment. Very belated update: We've been running with this on our clang/tot bots for a few weeks now and turned this on (or, more accurately stopped turning it off) for "normal" builds today. All tests pass and so far everything's happy. Repository: rG LLVM Github Monorepo CH

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

uabelho added a comment. In D87163#2373580 , @uabelho wrote: > Hi! > > I found a new problem after this was turned on again in 51ff04567b2f > . I wrote https://bugs.llvm.org/show_bug.cg

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

uabelho added a comment. Hi! I found a new problem after this was turned on again in 51ff04567b2f . With opt -S -o - bbi-49235.ll -memoryssa -gvn -dse -verify-memoryssa I hit opt: ../lib/Analysis/MemorySSA.cpp:2063: void

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. I put up an impromptu round-table to discuss MemorySSA related topics, including potential new transformations like MemCpyOpt using MemorySSA: https://whova.com/portal/webapp/llvm_202010/CommunityBoard/topic/342030/ If there's any interest, I think it would be great to sy

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. FYI this has been temporarily reverted again roughly a week ago, because there seems to be a mis-compile, that is caused by LLVM introducing invalid lifetime.end calls and exposed by the more powerful DSE, which @asbirlea is currently trying to narrow down. Repository:

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2285984 , @dmajor wrote: > I threw as many tests as I could find at 9d172c8e9c84 > , and I > don't see any regressions compared to its parent revision.

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. I threw as many tests as I could find at 9d172c8e9c84 , and I don't see any regressions compared to its parent revision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revie

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. Thanks for the heads up. I'll schedule a more intensive test run over the weekend while there's less demand for machines. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87163/new/ https://reviews.llvm.org/D87163 ___

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. All reported problems should be fixed now and we also made the fix for the issue @dmajor reported more conservative to catch more problematic cases. I just enabled DSE + MemorySSA again in 9d172c8e9c84 .

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2276899 , @dmajor wrote: > In D87163#2275896 , @asbirlea wrote: > >> I checked in a fix in https://reviews.llvm.org/rGfc8200633122, but I haven't >> yet verified it addresses all

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. In D87163#2275896 , @asbirlea wrote: > I checked in a fix in https://reviews.llvm.org/rGfc8200633122, but I haven't > yet verified it addresses all the failures reported. Thanks, I've confirmed that this fixes our tests in their o

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

asbirlea added a comment. I checked in a fix in https://reviews.llvm.org/rGfc8200633122, but I haven't yet verified it addresses all the failures reported. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87163/new/ https://reviews.llvm.org/D87163 _

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

asbirlea added a comment. Right! I believe the solution is to set the location size to unknown after a phi translation with the same ptr, but I need to properly verify that. // include/llvm/Analysis/MemorySSA.h:1233 CurrentPair.second = Location.getWithNewSize(LocationSize::unknown()); ret

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2275058 , @asbirlea wrote: > Yes, the load should have the Phi as the defining access. I'm still looking > into where this information should come from, but it's not hitting the phi > translation. > Thank you for the reve

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

asbirlea added a comment. Yes, the load should have the Phi as the defining access. I'm still looking into where this information should come from, but it's not hitting the phi translation. Thank you for the revert, I'll update with the fix once I have it available. Repository: rG LLVM Githu

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2274549 , @dmajor wrote: > Reduced a bit more: https://godbolt.org/z/j59evK (C++) and > https://godbolt.org/z/8xG688 (IR) -- the store at line 43 of `while.end` has > been removed. Thanks! I reverted the change for now i

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. Reduced a bit more: https://godbolt.org/z/j59evK (C++) and https://godbolt.org/z/8xG688 (IR) -- the store at line 43 of `while.end` has been removed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87163/new/ https://reviews

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. Here is a reduced-ish repro, I can keep poking at it but maybe this is sufficient for you to notice something: https://godbolt.org/z/endf6d. Center pane is old DSE, right pane is new DSE. I highlighted the problem area with a nop at line 56 of the source, and line 59 of

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2273917 , @thakis wrote: > It's not blocking us because we added an explicit flag to force this off to > our build config for "normal" builds. (We still get the default-on behavior > on our bots that build with trunk clan

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

thakis added a comment. It's not blocking us because we added an explicit flag to force this off to our build config for "normal" builds. (We still get the default-on behavior on our bots that build with trunk clang.) But if others are seeing problems with this too, I think it makes sense to re

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2272749 , @asbirlea wrote: > I'd suggest reverting if the failures are blocking, but we do need a > reproducer so it can be recommitted after a fix is in place. +1, but I think ideally we would have reproducers for the re

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

asbirlea added a comment. I'd suggest reverting if the failures are blocking, but we do need a reproducer so it can be recommitted after a fix is in place. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87163/new/ https://reviews.llvm.org/D87163 _

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. In D87163#2272105 , @fhahn wrote: > I also pushed a fix for a MemorySSA issue that was exposed through DSE + > MemorySSA in c4f1b3144184 > . This > might also f

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2271998 , @dmajor wrote: > In D87163#2270871 , @fhahn wrote: > >> In D87163#2270444 , @thakis wrote: >> >>> Heads-up: We're seeing fairly wid

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. In D87163#2270871 , @fhahn wrote: > In D87163#2270444 , @thakis wrote: > >> Heads-up: We're seeing fairly widespread test failures with this in Chromium >> that go away when we force this fl

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

uabelho added a comment. In D87163#2270918 , @uabelho wrote: > In D87163#2270860 , @fhahn wrote: > >> In D87163#2270568 , @uabelho wrote: >> >>> Hi, >>> >>> I'm seeing what I

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

uabelho added a comment. In D87163#2270860 , @fhahn wrote: > In D87163#2270568 , @uabelho wrote: > >> Hi, >> >> I'm seeing what I think is a miscompile with this: > > Thanks for the reproducer. We have to be more ca

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2270444 , @thakis wrote: > Heads-up: We're seeing fairly widespread test failures with this in Chromium > that go away when we force this flag off. It's possible that it's due to only > a small number of issues and they m

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2270568 , @uabelho wrote: > Hi, > > I'm seeing what I think is a miscompile with this: Thanks for the reproducer. We have to be more careful around pointers that may reference multiple locations in memory. I pushed f715d8

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

uabelho added a comment. Hi, I'm seeing what I think is a miscompile with this: opt -S -dse -o - dse.ll on @x = global [10 x [10 x i16]] zeroinitializer, align 1 define i16 @f() { entry: br label %do.body do.body: ; preds = %if.end,

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

thakis added a comment. Heads-up: We're seeing fairly widespread test failures with this in Chromium that go away when we force this flag off. It's possible that it's due to only a small number of issues and they might be previously-benign UB (no idea), but I figured I'd give an early note. We'

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2269674 , @thakis wrote: > The fix-up breaks tests: http://45.33.8.238/linux/27687/step_12.txt Thanks, should already be fixed in d85ac6d577ac Reposi

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

thakis added a comment. The fix-up breaks tests: http://45.33.8.238/linux/27687/step_12.txt Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87163/new/ https://reviews.llvm.org/D87163 ___ cfe-commits mailin

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGfb109c42d91c: [DSE] Switch to MemorySSA-backed DSE by default. (authored by fhahn). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2266725 , @asbirlea wrote: > I'm running additional testing in the background. I don't have anything > holding this back at this point. > Either check in and if something shows up, I'll let you know to resolve or > revert

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

asbirlea accepted this revision. asbirlea added a comment. This revision is now accepted and ready to land. I'm running additional testing in the background. I don't have anything holding this back at this point. Either check in and if something shows up, I'll let you know to resolve or revert,

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn updated this revision to Diff 291078. fhahn added a comment. Final rebase after new memcpy test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87163/new/ https://reviews.llvm.org/D87163 Files: clang/test/CodeGen/thinlto-distributed-newpm.l

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. In D87163#2264785 , @asbirlea wrote: > I'm running into a crash with this, could you please hold off until tomorrow? > I'm working on getting a reproducer. @asbirlea Thanks for the reproducer, the issue should be sorted out. The GV

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added inline comments. Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:312 switch (II->getIntrinsicID()) { default: llvm_unreachable("doesn't pass 'hasAnalyzableMemoryWrite' predicate"); case Intrinsic::lifetime_end: asbirlea

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

asbirlea added inline comments. Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:312 switch (II->getIntrinsicID()) { default: llvm_unreachable("doesn't pass 'hasAnalyzableMemoryWrite' predicate"); case Intrinsic::lifetime_end: The c

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

asbirlea requested changes to this revision. asbirlea added a comment. This revision now requires changes to proceed. I'm running into a crash with this, could you please hold off until tomorrow? I'm working on getting a reproducer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. It looks like a recently introduced GVN crash is exposed by this change (probably D87061 ). I will commit once this is fixed. In D87163#2263685 , @nikic wrote: > LGTM as well, per llvm-dev discussi

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

asbirlea accepted this revision. asbirlea added a comment. Green light from me as well. I think there's still room for improvement for the NPM, but this is the right step to move forward infrastructure-wise. Regarding the MemCpyOpt question from nikic@, please let me know if you want to take th

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

nikic accepted this revision. nikic added a comment. LGTM as well, per llvm-dev discussion. I think you've done all the due diligence we can expect. Do you plan to also work on MSSA-based MemCpyOpt? I expect that one will have a large positive impact on Rust code, where lack of cross-BB memcpy

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

xbolva00 accepted this revision. xbolva00 added a subscriber: spatel. xbolva00 added a comment. This revision is now accepted and ready to land. Thanks, amazing work! I agree that this should be enabled now and do not wait anymore. @nikic @spatel ? Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. As another data point, I have collected compile-time numbers for ARM64 with -Os -g LTO & -Oz -g LTO. Geoman changes below. Compile time here is actual execution time on a stabilized system. Compile time code size -Oz RLTO 0.67%

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn updated this revision to Diff 290658. fhahn added a comment. rebase on top of latest changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87163/new/ https://reviews.llvm.org/D87163 Files: clang/test/CodeGen/thinlto-distributed-newpm.ll

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added a comment. Note that we now also have to compute the PostDominatorTree, which adds an extra bit of compile-time overhead. By adjusting the pipeline a bit more, we can re-use ADCE's PDTs in most cases, which gives a `-0.18%` geomean improvement for -O3 D87322

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn added inline comments. Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:115 cl::desc("The number of memory instructions to scan for " "dead store elimination (default = 100)")); static cl::opt MemoryS

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn updated this revision to Diff 290140. fhahn added a comment. Adjust limit in description, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87163/new/ https://reviews.llvm.org/D87163 Files: clang/test/CodeGen/thinlto-distributed-newpm.

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

xbolva00 added inline comments. Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:115 cl::desc("The number of memory instructions to scan for " "dead store elimination (default = 100)")); static cl::opt Memo

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

fhahn created this revision. fhahn added reviewers: efriedma, dmgreen, asbirlea, reames. Herald added subscribers: cfe-commits, lxfind, nikic, kerbowa, jfb, dexonsmith, steven_wu, george.burgess.iv, modocache, hiraditya, Prazek, nhaehnle, jvesely. Herald added projects: clang, LLVM. fhahn requeste