Feature/initialize list side branch #6058
Conversation
kaizhangNV
requested review from
tangent-vector,
csyonghe,
expipiplus1 and
a team
as code owners
kaizhangNV
removed request for
a team,
expipiplus1,
csyonghe and
tangent-vector
kaizhangNV
force-pushed
the
feature/initialize-list-side-branch
branch
12 times, most recently
from
282ad96 to
8be343e
Compare
kaizhangNV
force-pushed
the
feature/initialize-list-side-branch
branch
6 times, most recently
from
2f3c992 to
7c5a8e8
Compare
Don't report error when checking synthesized constructor, we will remove this constructor instead. Change the location where we create the member initialize constructor signature to SemanticsDeclAttributesVisitor to avoid the circular checking problem.
1. When extension of a struct defines __init, we should not synthesize __init anymore. 2. For the synthesized __init signature, we correct the default value for the parameter. Figuring out what exactly is "default initialize type". If a type has a ctor can take 0 parameter, it's default initialize type.
Struct Inheriting from a interface doesn't violate the rules of C-Style struct. Add the visibility modifier to the synthesized ctor.
We need to ensure the definite checked for a struct before trying to use its constructor in the initialize list coerce.
In ResolveInvoke() call, we add a special case for one-argument Invoke resolution. Because slang will treat this as a type conversion Type1 A; Type2 A = Type2(B); We we synthesize the constructor invocation, we have to bypass this special handling, because this invocation is converted from initialize list, so want to call the constructor directly.
We should not use parameter's default value in the synthesized member-init ctor. The default value should only be used when call site doesn't provide enough arguments, and our invoke system will use those default value to make up the missing arguments. Add '-vk' to few tests to enable vulkan backend test.
Fix gfx-smoke test, for non-C style struct,
StructType a = {}; will never work anymore.
-Formatting
…ructor When successfully check the synthesized member initializer list constructor, we don't need the default constructor anymore, so we can safely remove it.
Disable -zero-initialize option and disable all the related tests. File shader-slang#6109 to track this issue.
Add cuda-host decoration for the synthesized constructor if there is any usage of TorchTensor.
kaizhangNV
force-pushed
the
feature/initialize-list-side-branch
branch
from
39cadb9 to
db3063a
Compare
When creating invoke expression for ctor, we need to call ResolveInvoke() to find us the best candidates, however the existing lookup logic could find us the base constructor for child struct, we should eliminate this case by providing the LookupOptions::IgnoreInheritance to lookup, this requires us to create a subcontext on SemanticsVisitor to indicate that we only want to use this option on looking the constructor. This change implements this.
| @@ -249,13 +249,25 @@ struct CheckDifferentiabilityPassContext : public InstPassBase | |||
| if (outerFuncInst->findDecoration<IRTorchEntryPointDecoration>()) | |||
| return; | |||
|
|
|||
| bool isSynthesizeConstructor = false; | |||
There was a problem hiding this comment.
Instead of working around here, I think the right solution may just be that we don't synthesize any constructors for types that have __intrinsic_type modifier on it, since these types maps t o special IR types and synthesized constructors for these types are not meaningful.
This way we wouldn't be synthesizing any constructor for the TorchTensor type, and won't run into this problem.
There was a problem hiding this comment.
This is not just handling that case, if there is a struct having a TorchTensor type members, we will still have to synthesize constructor for that struct.
There was a problem hiding this comment.
I see, this really starting to get into serious heterogeneous programming models if we need to be correct. I can see us using this workaround for now until we get better support for heterogeneous code.
| @@ -2175,7 +2175,8 @@ void SemanticsVisitor::AddTypeOverloadCandidates(Type* type, OverloadResolveCont | |||
| type, | |||
| context.sourceScope, | |||
| LookupMask::Default, | |||
| LookupOptions::NoDeref); | |||
| context.checkForSynthesizedCtor ? LookupOptions::IgnoreInheritance | |||
There was a problem hiding this comment.
why does this need to be conditional on checkForSynthesizedCtor, instead of just always ignore inheritance for all constructor lookups?
There was a problem hiding this comment.
Though I'm not 100% sure, it could be the reason on the comment, this lookup is also used by the coerce logic.
e.g.:
float3 <= vector<half, 3>
with only LookupOptions::IgnoreInheritance, the coerce could fail.
There was a problem hiding this comment.
I think maybe this is a bug but I thought it's valid,
e.g.
struct Foo
{
float3 a;
__init(float3 a) {this.a = a;}
}
float16_t3 x;
Foo f = Foo(x);
Is this a valid syntax? Can float16_t3 implicitly converts to float3?
There was a problem hiding this comment.
The reason I'm asking is because currently Falcor is using such syntax in their code base, but we don't treat this like an error. So that's why I'm thinking this is a valid syntax.
There was a problem hiding this comment.
I wonder why it will fail. These conversions should be defined in extensions and not in base types. IgnoreInheritance shouldn't be skipping ctors from extensions, right?
There was a problem hiding this comment.
Assuming this
Foo f = {x};
it will look up the explicit ctor, and it will find it, then we will coerce each argument to parameter type,
so coerce will trigger this AddTypeOverloadCandidates call for type float3. If the option is still LookupOptions::IgnoreInheritance, the coerce will fail.
But unfortunately, I don't know the reason yet.
There was a problem hiding this comment.
I think it is worth finding out, because float3 does not have a base type defines float3.__init(float16t_3), and such constructors should be discoverable even with IgnoreInheritance.
A possible reason is that the ctor is defined in a generic extension on BuiltinArithmeticType, and somehow IgnoreInheritance is causing us to ignore extensions defined on some base interface type.
There was a problem hiding this comment.
Yes, I think the root cause is that it prevent us from find those ctors defined in extension
they'are defined in core module
sb << " __implicit_conversion(" << cost << ")\n";
sb << " __intrinsic_op(" << int(op) << ")\n";
sb << " __init(vector<" << kBaseTypes[ff].name << ",N> value);\n";
There was a problem hiding this comment.
And I did some cross validation, revert my change, and look at the the best candidate.
There was a problem hiding this comment.
Are you saying IgnoreInheritance is ignoring all extensions? If so, we need a different option that is ignoring just the base types.
No description provided.