Page MenuHomePhabricator

Don't build jobs for the same Action + ToolChain twice.
ClosedPublic

Authored by jlebar on Jan 7 2016, 11:28 AM.

Details

Reviewers
echristo
jlebar
Summary

Right now if the Action graph is a DAG and we encounter an action twice,
we will run it twice.

This patch is difficult to test as-is, but I have testcases for this as
used within CUDA compilation.

Diff Detail

Event Timeline

jlebar updated this revision to Diff 44237.Jan 7 2016, 11:28 AM
jlebar retitled this revision from to Don't build jobs for the same Action + ToolChain twice..
jlebar updated this object.
jlebar added a reviewer: echristo.
jlebar added a subscriber: cfe-commits.
jlebar added a comment.Jan 7 2016, 7:31 PM

This actually has a subtle issue not found with existing unit tests: BuildJobsForAction has an outparam and we don't set it on cache hit.

Please hold off reviewing this until I fix the problem.

jlebar removed a reviewer: echristo.Jan 7 2016, 7:31 PM
jlebar added a subscriber: echristo.
jlebar updated this revision to Diff 44377.Jan 8 2016, 2:09 PM

Fixing bug caused by missing an outparam.

Covered by tests in WIP CUDA+ptxas+fatbin patch.

Depends on D16013.

jlebar removed a subscriber: echristo.

OK, this is now working, please have a look. I'm not sure if it's possible to write a test as-is, but I have a test for my mistake in my WIP CUDA patch. (Also this mistake is much harder to make after D16013.)

echristo edited edge metadata.Jan 11 2016, 2:49 PM

So, how are you getting to the point where you're trying to create the same action twice?

-eric

tra edited edge metadata.Jan 11 2016, 2:52 PM
tra added a subscriber: tra.

So, how are you getting to the point where you're trying to create the same action twice?

-eric

In the new CUDA world, we have the following graph, which I hope will render properly:

foo.cu --> foo.s (PTX) --> foo.cubin --> foo.fatbin
               └-----------------------┙

That is, foo.s is an input to foo.cubin *and* an input to foo.fatbin.

The Driver stores each Action's inputs. So starting from the fatbin, we look at its two inputs, and try to create jobs for them. Fine. Then we look at the input to the cubin. That's foo.s, which we already visited.

This comment was removed by jlebar.
jlebar accepted this revision.Jan 15 2016, 5:53 PM
jlebar added a reviewer: jlebar.

Pushed in r257808 with echristo's lg (sorry, didn't have the right metadata in the commit).

This revision is now accepted and ready to land.Jan 15 2016, 5:53 PM
jlebar closed this revision.Jan 15 2016, 5:53 PM