This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Avoid needing to walk out legalization tables. NFCI.
ClosedPublic

Authored by niravd on Jun 8 2018, 1:39 PM.

Details

Summary

To avoid redundant work, during DAG legalization we keep tables
mapping pre-legalized SDValues to post-legalized SDValues and a
SDValue-to-SDValue map to enable fast node replacements. However, as
the keys are nodes which may be reused it is possible that an entry in
a table refers to a now deleted node N (that should have been renamed
by the Value replacement map) while a new node N' exists. If N' is
then replaced that entry would be wrong. Previously we avoided this by
when potentially violating this property, walking every table and
updating all node pointers. This is very expensive but hopefully rare
occurance.

This patch assigns each instance of a SDValue used in legalization a
unique id and uses these ids in the legalization tables. This avoids
any such aliasing issue, avoiding the full table search and allowing
more aggressive incremental table pruning.

In some cases this is a 1000x speedup to compilation.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd created this revision.Jun 8 2018, 1:39 PM
grandinj added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
160 ↗(On Diff #150560)

cheaper just to return NextValueId-1 here?

Also, if you want to catch wraparound, it's a good place to stick

assert(NextValueId !=0 && "wraparound, need to implement compactifcation")
niravd updated this revision to Diff 150793.Jun 11 2018, 11:01 AM

Minor clean up and address Noel's comments.

dberris added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
98 ↗(On Diff #150793)

s/compactifcation/compaction/ ?

Adding Justin as I'm more comfortable with him reviewing this than me :)

jlebar edited reviewers, added: tra; removed: jlebar.Jun 13 2018, 2:21 PM
bogner accepted this revision.Jun 13 2018, 2:37 PM

A couple of very minor nitpicks about use of auto, below, but overall this looks very nice. LGTM.

llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
102 ↗(On Diff #150793)

Better to use auto for iterators.

llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
259–260 ↗(On Diff #150793)

I'd probably spell out the types here - it isn't any longer and it's arguably slightly easier to read. Similarly in a few places below, especially in the cases that currently mix using auto and spelling out SDValue.

This revision is now accepted and ready to land.Jun 13 2018, 2:37 PM
This revision was automatically updated to reflect the committed changes.
niravd marked an inline comment as done.

This commit breaks the bots with expensive checks on:

I can also reproduce locally with just:

./build/bin/llc -mtriple=x86_64-- test/CodeGen/X86/base-pointer-and-cmpxchg.ll -o -

Can you please take a look? Thanks!