Page MenuHomePhabricator

Avoid combining adjacent stores at -O0 to improve debug experience
ClosedPublic

Authored by probinson on Jan 26 2015, 10:30 AM.

Details

Summary

When the source has a series of assignments, users reasonably want to
have the debugger step through each one individually. Turn off the combine
for adjacent stores so we get this behavior at -O0.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson updated this revision to Diff 18770.Jan 26 2015, 10:30 AM
probinson retitled this revision from to Avoid combining adjacent stores at -O0 to improve debug experience.
probinson updated this object.
probinson edited the test plan for this revision. (Show Details)
probinson added reviewers: echristo, dblaikie.
probinson added a subscriber: Unknown Object (MLST).
emaste added a subscriber: emaste.Jan 26 2015, 10:54 AM
echristo edited edge metadata.Jan 26 2015, 11:01 AM

Arguably no part of the DAG Combiner should be running?

Now that you mention it, I see we do disable all of DAGCombiner for optnone;
I'll hoist the -O0 check up there too and see what dies.

If I disable DAGCombine at -O0, I get 19 test failures including two crashes.
Abstractly it seems reasonable to do that, or at least identify and separate out the combines that have to run even at -O0; but we're not there now, and I'm probably not the right person to figure that stuff out.
How about I commit the patch as is, and file a bug for disabling DAGCombine at -O0?

There should be exactly zero DAG combines that have to run, ever. They are by definition not required for correctness. Anything else is a bug, and a bad one.

Okay, file a bug *and* put a FIXME on this patch? That will at least solve my
debug-info issue until the rest of the DAGCombine thing gets sorted out.

Filed PR22346... disabling all DAGCombine at -O0 does not have consensus, it seems, and is certainly not feasible without doing an unknown but clearly nontrivial amount of work first.

Can I get an LGTM on the patch as proposed? Thanks!

This revision was automatically updated to reflect the committed changes.

r230659, thanks!