This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Use thread-pool's notion of thread count instead of requerying system.
ClosedPublic

Authored by stellaraccident on Dec 23 2021, 4:19 PM.

Details

Summary

The computed number of hardware threads can change over the life of the process based on affinity changes. Since we need a data structure that is at least as large as the maximum parallelism, it is important to use the value that was actually latched for the thread pool we will be dispatching work to.

Also adds an assert specifically for if it doesn't line up (I was getting a crash on an index into the vector).

Diff Detail

Event Timeline

stellaraccident requested review of this revision.Dec 23 2021, 4:19 PM
mehdi_amini added inline comments.Dec 23 2021, 4:21 PM
mlir/lib/Transforms/Inliner.cpp
705
mehdi_amini accepted this revision.Dec 23 2021, 4:24 PM
mehdi_amini added inline comments.
mlir/lib/Transforms/Inliner.cpp
680

Patch LGTM, but I don't quite get this comment right now.

This revision is now accepted and ready to land.Dec 23 2021, 4:24 PM

Remove comment.

mehdi_amini added inline comments.Dec 23 2021, 4:32 PM
mlir/lib/Transforms/Inliner.cpp
680

(I meant that I don't get the existing comment in the codebase)

stellaraccident marked an inline comment as done.Dec 23 2021, 4:33 PM
stellaraccident added inline comments.
mlir/lib/Transforms/Inliner.cpp
680

I was trying to indicate that there is an action at a distance constraint, but I couldn't come up with a better way to say it so just removed.

Mogball accepted this revision.Dec 23 2021, 4:34 PM

Good catch. I think this is the only use of compute_thread_count in MLIR core.

mlir/lib/Transforms/Inliner.cpp
680

How about:

"We must maintain a fixed pool of pass managers which is at least as large as the maximum parallelism of the failableParallelForEach below."

I don't understand the instrumentation/main thread connection myself.

Comment.

mlir/lib/Transforms/Inliner.cpp
680

I went ahead and kept the Note. I don't know quite what it is trying to convey but seems important to understand in a followup.

mehdi_amini added inline comments.Dec 23 2021, 5:04 PM
mlir/lib/Transforms/Inliner.cpp
680

Yeah it's the part about "The number of pass managers here needs to remain constant" that puzzles me, since we resize it right below.

705

(did you miss this one?)

This revision was landed with ongoing or failed builds.Dec 23 2021, 5:05 PM
This revision was automatically updated to reflect the committed changes.
mlir/lib/Transforms/Inliner.cpp
705

It shows as changed in my snapshot.

Filed issue: https://github.com/llvm/llvm-project/issues/52862 for clarification. I don't like not understanding action-at-a-distance things like this and maybe the author can recall.