This is an archive of the discontinued LLVM Phabricator instance.

DSE: Don't remove stores made live by a call which unwinds.
ClosedPublic

Authored by eli.friedman on Jun 5 2016, 3:54 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

eli.friedman retitled this revision from to DSE: Don't remove stores made live by a call which unwinds..
eli.friedman updated this object.
eli.friedman added a subscriber: llvm-commits.
hfinkel added inline comments.Jun 5 2016, 4:24 PM
lib/Transforms/Scalar/DeadStoreElimination.cpp
860 ↗(On Diff #59680)

Not clear that OrderedBasicBlock will help here directly. I think you really want two maps:

  1. Instruction* -> unsigned (for ordering)
  2. Instruction* -> Instruction* (or unsigned) (for the next call that might throw)

Why not just collect the maps above (where you currently set MayThrow), and then answer the question precisely? I think it is likely I'll see performance regressions from this if we over-approximate.

867 ↗(On Diff #59680)

Inst here should really be the first call that might throw, right?

eli.friedman added inline comments.Jun 5 2016, 4:52 PM
lib/Transforms/Scalar/DeadStoreElimination.cpp
860 ↗(On Diff #59680)

Yes, I can do something like that. Not sure the second map is necessary: I think we just need the most recently seen unwinding call in the block?

867 ↗(On Diff #59680)

Yes; I would probably call it the most recently seen call that might throw.

hfinkel added inline comments.Jun 5 2016, 6:58 PM
lib/Transforms/Scalar/DeadStoreElimination.cpp
860 ↗(On Diff #59680)

Ah, yes. You're right. Thanks!

  • [DSE] Precisely track instructions which may unwind.

Updated. Unfortunately, the patch gets a lot more complicated because of the need to avoid iterator invalidation. On the plus side, I think I fixed a couple latent iterator invalidation bugs.

mcrosier edited edge metadata.Jun 21 2016, 11:16 AM

The mechanical changes to deal with the iterators LGTM. I'll defer to Hal or someone else to approve the changes to remove stores made live by a call which unwinds.

lib/Transforms/Scalar/DeadStoreElimination.cpp
90 ↗(On Diff #59689)

Should we keep the comments from line 54?

837 ↗(On Diff #59689)

pre-increment InstrIndex.

892 ↗(On Diff #59689)

Space between 'assert' and right paren.

Perhaps, a clang-format?

Hi @eli.friedman,
I believe I've hijacked all I'm going to, so you might consider rebasing the patch on top of the fresh DSE updates. Hopefully, this patch is now fairly simple and thanks for your assistance.

Chad

eli.friedman edited edge metadata.

Rebase on top of trunk.

dberlin added inline comments.
lib/Transforms/Scalar/DeadStoreElimination.cpp
1066 ↗(On Diff #65022)

(and fwiw, memdep does not consider maythrow, only aliasing. AFAIK, it will never return a maythrow call as a dep unless it thinks, conservatively, that it aliases the pointer. It mostly does think that when we don't have good call mod/ref info, but when we do, it happily does not return maythrow calls as deps)

1073 ↗(On Diff #65022)

PointerMayBeCapturedBefore is an O(N) call, at best.
It even says " This routine can be expensive, so consider
/// caching the results. "

Can we really not do better?

828 ↗(On Diff #59689)

Is this different from orderedbasicblock?

InstrOrdering is basically an OrderedBasicBlock, I guess... I think I'd need to modify the interface a bit for this exact use to deal with modifications.

lib/Transforms/Scalar/DeadStoreElimination.cpp
1073 ↗(On Diff #65022)

We could try to cache the result, I guess... but I don't think we would end up hitting any such cache very often, considering that you can only reuse the cache for exactly the same inputs.

I could use PointerMayBeCaptured instead. Not quite as strong, but I guess the extra accuracy might not be that useful (and it's easier to cache, if necessary).

Don't use PointerMayBeCapturedBefore. Add some comments.

This revision was automatically updated to reflect the committed changes.