Page MenuHomePhabricator

[DAG,X86] Improve Dependency analysis when doing multi-node Instruction Selection
ClosedPublic

Authored by niravd on Dec 15 2017, 8:34 AM.

Details

Summary

Cleanup cycle/validity checks in ISel (IsLegalToFold, HandleMergeInputChains) and X86 (isFusableLoadOpStore).
Now do a full search for cycles / dependencies pruning the search when topological property of NodeId allows.

As part of this propogate the NodeId-based cutoffs to narrow hasPreprocessorHelper searches.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd created this revision.Dec 15 2017, 8:34 AM
niravd retitled this revision from [DAG] Improve multi-chain merging in ISel to [DAG] Improve Dependency analysis when doing multi-node Instruction Selection.Dec 18 2017, 1:41 PM
niravd updated this revision to Diff 128536.Jan 3 2018, 10:25 AM
niravd edited the summary of this revision. (Show Details)

Rebase and fix NodeId pruning to also ignore 0 ids (from legalization) and turn pruning on for checks only

craig.topper added inline comments.Jan 4 2018, 12:50 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
2135 ↗(On Diff #128536)

There's no longer a parameter named "Use" so the description doesn't make sense anymore

2399 ↗(On Diff #128536)

Add curly braces around this body for readability's sake

2416 ↗(On Diff #128536)

I think this Visited.clear call is indented incorrectly?

llvm/test/CodeGen/X86/2012-01-16-mfence-nosse-flags.ll
17 ↗(On Diff #128536)

Why did we fail to use testl here now?

llvm/test/CodeGen/X86/subvector-broadcast.ll
922 ↗(On Diff #128536)

There's seems to be a bug in the execution domain vbroadcastf128 where its being reported as Int instead of FP which is causing the vmovaps/vmovdq difference between AVX1 and AVX2. I'm going to fix that so you'll probably need to rebase this area.

niravd added inline comments.Jan 5 2018, 8:42 AM
llvm/test/CodeGen/X86/2012-01-16-mfence-nosse-flags.ll
17 ↗(On Diff #128536)

ISel now can merge the load with the compare operation. We later split it in a call to unfoldMemoryOperand but we only do the check to covert compares back to tests in the MI verstion not the DAG version that's being called.

niravd updated this revision to Diff 129332.Jan 10 2018, 1:43 PM
niravd marked 4 inline comments as done.

Address comments and rebase.

RKSimon added a subscriber: RKSimon.

A couple of (very) minor observations

llvm/include/llvm/CodeGen/SelectionDAGNodes.h
824 ↗(On Diff #129332)

(MId < NId)

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
2135 ↗(On Diff #129332)

Root

niravd updated this revision to Diff 129462.Jan 11 2018, 9:27 AM

Minor cleanup.

niravd marked 2 inline comments as done.Jan 11 2018, 9:27 AM
niravd updated this revision to Diff 129669.Jan 12 2018, 11:16 AM
niravd retitled this revision from [DAG] Improve Dependency analysis when doing multi-node Instruction Selection to [DAG,X86] Improve Dependency analysis when doing multi-node Instruction Selection.
niravd edited the summary of this revision. (Show Details)

In testing I discovered the follow on patch to X86's load-op-store pattern check needed to be folded in. Added discovered test as well.

niravd updated this revision to Diff 130019.Jan 16 2018, 1:08 PM

Rebasing past D42128.

Apart from a very minor tweak, no more comments - someone who knows this code better should give it the once over if possible.

llvm/include/llvm/CodeGen/SelectionDAGNodes.h
817 ↗(On Diff #130019)

Maybe better to use int instead of auto - the type isn't obvious? Same for MId below.

niravd updated this revision to Diff 130947.Jan 22 2018, 12:27 PM
niravd marked an inline comment as done.

Address RKSimon's comments and add trivial early exit check in findNonImmUse.

niravd updated this revision to Diff 131138.Jan 23 2018, 1:24 PM

Found bug when ImmedUse has non-direct uses of Def. Added test and fixup.

niravd updated this revision to Diff 131488.Jan 25 2018, 11:24 AM

Turn back on nodeid-based pruning on X86 Target.

jyknight added inline comments.
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
804 ↗(On Diff #131488)

Should document what the new argument is for in function comment.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
2257 ↗(On Diff #131488)

Still mentions WalkChainUsers, but that function is deleted.

2439 ↗(On Diff #131488)

This can move up immediately after the call to AddChains, right?

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
2111 ↗(On Diff #131488)

Rename ChainCheck "FoundLoad".

2128 ↗(On Diff #131488)

if (!FoundLoad) return false; and unindent the rest.

llvm/test/CodeGen/X86/2012-01-16-mfence-nosse-flags.ll
17 ↗(On Diff #128536)

Put that into a comment maybe?

niravd updated this revision to Diff 131815.Jan 29 2018, 9:05 AM
niravd marked an inline comment as done.

address reviewer comments

niravd updated this revision to Diff 132041.Jan 30 2018, 1:40 PM
niravd edited the summary of this revision. (Show Details)

Update because of PR35316. Also tweak findNonImmUse to use hasPredecessorHelper like other functions.

niravd updated this revision to Diff 132183.Jan 31 2018, 8:33 AM
niravd edited the summary of this revision. (Show Details)

Failed to commit typo fix (N vs. M) in last rebase.

niravd edited the summary of this revision. (Show Details)Jan 31 2018, 11:45 AM
bogner accepted this revision.Feb 5 2018, 10:36 AM

Seems reasonable. A couple of nitpicks.

llvm/include/llvm/CodeGen/SelectionDAGNodes.h
808 ↗(On Diff #132183)

16 seems kind of large for the SmallVector, given that in all the cases we call this with TopologicalPrune=false will leave this empty.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
2177 ↗(On Diff #132183)

Extra set of parens?

This revision is now accepted and ready to land.Feb 5 2018, 10:36 AM
This revision was automatically updated to reflect the committed changes.