Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Importer] Preserve output PH names in C2 loader #3798

Closed
wants to merge 1 commit into from

Conversation

@nickgg
Copy link
Contributor

nickgg commented Nov 20, 2019

Summary: It's important to preserve the name of each external output in outside models, but since they must be unique with Node names we'll need to claim them before naming any nodes. This fixes the C2 Model Loader to do that.

Documentation: Fixes #3774

Test Plan: ninja test, image-classifier with -dump-ir to make sure weights were named right.

@mciprian13

This comment has been minimized.

Copy link
Contributor

mciprian13 commented Nov 20, 2019

I assume also for the Resnet50 example in main.cpp (line 303) the symbol "gpu_0_softmax__1" should be changed to "gpu_0_softmax".

@mciprian13

This comment has been minimized.

Copy link
Contributor

mciprian13 commented Nov 20, 2019

I was wondering whether we should add unit tests to run the generated bundles (now -I think - the CI only verifies they build/compile properly).

@nickgg nickgg force-pushed the nickgg:fixC2OutputName branch 2 times, most recently from 0367910 to 41ae540 Nov 20, 2019
@@ -109,6 +109,10 @@ class Module final {
usedNodeNames_.insert(name);
}

void registerStorageName(llvm::StringRef name) {

This comment has been minimized.

Copy link
@bertmaher

bertmaher Nov 20, 2019

Contributor

Doxygen

@@ -1464,8 +1471,13 @@ Error Caffe2ModelLoader::loadNetwork(caffe2::NetDef &net) {
auto &outputName = net.external_output(i);
NodeValue r;
ASSIGN_VALUE_OR_RETURN_ERR(r, getNodeValueByName(outputName));

This comment has been minimized.

Copy link
@bertmaher

bertmaher Nov 20, 2019

Contributor

I don't understand something here: if nodes and placeholders share a namespace, wouldn't this getNodeValueByName fail to find the desired node, since it's probably been re-named something like output__1?

This comment has been minimized.

Copy link
@nickgg

nickgg Nov 20, 2019

Author Contributor

Good question. It clearly works for e.g resnet, but your explanation sounds right. I'll look into it.

This comment has been minimized.

Copy link
@nickgg

nickgg Nov 21, 2019

Author Contributor

OK, so the way this works is getNodeValueByName has its own map (ie not the Function/Module) which claiming a name in the Module doesn't affect. This is good because we're looking up illegal names directly, and double good because we write into that name without checking if the name is already taken. TLDR: nodeValueByName uses different names than the Nodes themselves.

@nickgg nickgg force-pushed the nickgg:fixC2OutputName branch 2 times, most recently from c7a082b to b835b8d Nov 20, 2019
Copy link

facebook-github-bot left a comment

@nickgg has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

bertmaher left a comment

Thanks for the explanation! Looks great.

@jfix71
jfix71 approved these changes Nov 21, 2019
Copy link
Contributor

jfix71 left a comment

Cool, makes sense to me!

@@ -1444,6 +1444,13 @@ Error Caffe2ModelLoader::loadInputs(const caffe2::NetDef &net,
}

Error Caffe2ModelLoader::loadNetwork(caffe2::NetDef &net) {

/// Make a claim on the unique name of all output Placeholders.

This comment has been minimized.

Copy link
@jfix71

jfix71 Nov 21, 2019

Contributor

nit: //

@nickgg nickgg force-pushed the nickgg:fixC2OutputName branch from b835b8d to 1fa2592 Dec 2, 2019
Copy link

facebook-github-bot left a comment

@nickgg is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.