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

DFCxx kernel instantiation #52

Merged
merged 7 commits into from
Dec 2, 2024
Merged

DFCxx kernel instantiation #52

merged 7 commits into from
Dec 2, 2024

Conversation

Muxianesty
Copy link
Collaborator

No description provided.

@Muxianesty Muxianesty added enhancement New feature or request stage II For issues applicable to Stage II of the project labels Nov 19, 2024
@Muxianesty Muxianesty self-assigned this Nov 19, 2024
@@ -28,6 +28,10 @@ class IO {

DFVariable inputScalar(const std::string &name, const DFType &type);

DFVariable hollow(const DFType &type);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "hollow" synonym here? What does this method do exactly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an unconnected node without specific operation behind it. Currently it can only be used to be bound to another kernel's output. By the end of kernel's construction there mustn't be any nodes like that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hollow -> unconnected?
hollow -> dummy?

It would be good to have more describing name for the method here

Copy link
Collaborator Author

@Muxianesty Muxianesty Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it, but Unconnected - too long; Dummy - too confusing. I am going to check MaxCompiler docs on how they call such things.

UPD: MaxCompiler has DFEType::newInstance methods which do the same. I do not want to associate this functionality (at least now) with types, and instead treat them as kernel methods. Perhaps just newStream and newScalar?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, newStream and newScalar look better

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Comment on lines 126 to 149
if (it->target == output) {
if (target.type == OpType::NONE) {
outs.erase(it);
for (auto &out: outputs[target]) {
for (auto &in: inputs[out.target]) {
if (in.source == target && out == in) {
in.source = it->source;
outs.push_back(in);
}
}
auto conIt = connections.find(out.target);
if (conIt != connections.end() && conIt->second.source == target) {
conIt->second.source = it->source;
}
}
target = it->source;
} else {
it->target = target;
inputs[target].clear();
inputs[target].push_back(*it);
connections[target] = *it;
}
break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code fragment is too "nesting". Please decrease it, using "continue" operator inside loop, etc.

@Muxianesty Muxianesty marked this pull request as draft November 19, 2024 14:38
Copy link
Collaborator

@ssmolov ssmolov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@ssmolov ssmolov marked this pull request as ready for review December 2, 2024 14:40
Copy link
Collaborator

@ssmolov ssmolov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems ok

@ssmolov ssmolov merged commit 77ec3eb into master Dec 2, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stage II For issues applicable to Stage II of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants