This is an archive of the discontinued LLVM Phabricator instance.

[DSE] Improve mayThrowBetween for 2 instructions in the same BB.
AbandonedPublic

Authored by fhahn on Jan 6 2020, 12:46 PM.

Details

Summary

This patch adds a way to quickly check if there are throwing instructions
between two instructions in the same basic block.

To do so, we first number all instructions in a BB starting from 0 and
incrementing the counter every time we hit a throwing instruction. If
2 instructions have the same number, there is no throwing instruction
between them.

Diff Detail

Event Timeline

fhahn created this revision.Jan 6 2020, 12:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2020, 12:46 PM
Herald added subscribers: jfb, hiraditya. · View Herald Transcript

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

efriedma added inline comments.Jan 6 2020, 2:19 PM
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1644

What happens if NI is the first instruction in the block? Or is that impossible in this context?

1706

This map seems sort of expensive, but maybe not a big deal relative to other stuff we have here already.

fhahn marked 2 inline comments as done.Jan 7 2020, 11:22 AM
fhahn added inline comments.
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1644

I think that should be impossible, as SI must come before NI (they cannot be equal either) and SI and NI must be in the same BB.

1706

Yep, we probably should populate it lazily. And I think at least for C/C++ programs, it should not really be needed at all. There's no difference in the MultiSource/SPEC2000/SPEC2006 binaries with and without this patch, now that I updated D72146 to skip throwing instructions modeled in MemorySSA.

efriedma added inline comments.Jan 7 2020, 12:53 PM
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1706

That's not surprising. It's theoretically possible to have a function that's readnone and not nothrow, but it doesn't happen in practice for C/C++ code.

fhahn planned changes to this revision.Jan 7 2020, 1:12 PM
fhahn marked an inline comment as done.

This is not really required for C/C++. I'll re-visit the patch once we have the more important pieces of MemorySSA based machinery in.

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1706

Yep, I think wit would probably make sense to re-visit this patch later and focus on getting the other parts in first :)

fhahn abandoned this revision.Jul 1 2022, 7:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2022, 7:24 AM