Member-wise constructor generation with visibility control #4854
Conversation
NOTE: This is a draft Key Changes: 1. Initializer lists call into constructors (and no longer hack around per element) 2. we auto-generate member-wise constructors for varying visibility of members (public, public-internal, public-internal-private) 3. Implements visibility and member-wise constructor rules described in shader-slang#3406 4. Reorders and reformats how we auto-generate constructors along with adding support for member-wise constructors ('visitStruct' and 'visitAggTypeDecl'). * This was changed since currently Slang (if no constructors are found) falls back to initializer list syntax for non initializer list constructor code. Since we add a non-default-ctor this fallback logic never happens causing failures now. 5. initialization-list logic for struct objects has been reordered and reformatted due to previous logical incompatibilities.
…lG-NV/slang into initializer-list-visibility
…lG-NV/slang into initializer-list-visibility
1. resolve generics which are associated to a struct instance during init list evaluation 2. fix autodiff test with incorrect init-list
…lG-NV/slang into initializer-list-visibility
…rrent limitations." Revert because it will break code, instead just check more generally for 0, if we don't want the "everything does an 'init'" logic this can be changes later.
…iures remove all null default-ctor's like before, instead though redesign the hacky overload resolution into ctor such that it works: 1. resolveInvoke properly culls useless overloads 2. This stops spirious generation of empty init's (side-effect if we generate a real ctor) 3. This stops lots of warnings since we don't have a ctor that init's nothing
1. allow a more formalized init-list logic for flattened init-lists's 2. fixing invalid tests
1. clean-up documentation tests 2. fix recursive type crash with uninitialized value checks 3. fully disallow synth object printing rather than partially for auto-documentation code.
|
Note: to repro the Note: |
1. fix auto-diff issues that cause incorrect `no_diff` error when Slang should not error 2. change when Slang applies to auto-gen parameters `no_diff` to be more correct/accurate
…o better report compile failiures with hlsl
…lG-NV/slang into initializer-list-visibility
|
Note: Falcor is using a c-style-init-list with a non c-style-struct ( |
|
Note: any commits after this comment will include "Legacy Slang init-list support" to avoid breaking legacy Slang code |
…lG-NV/slang into initializer-list-visibility
| Footprint footprint = {__queryFootprint$(CoarseOrFine)NVAPI( | ||
| Shape.dimensions, | ||
| shapeDim, |
There was a problem hiding this comment.
It seems like this may be due to the lookup scope of the expr isn't correct hooked up when you create the ctor call expr?
There was a problem hiding this comment.
This is very likely
|
|
||
| // `vector<T,N>` and `vector<T,U>` may not be equal types, but, we have no choice in this senario but to assume coerce logic | ||
| // can resolve such situations, otherwise we need to fail | ||
| if (isSameSimpleExprType(toType, fromExpr->type)) |
There was a problem hiding this comment.
how can this not implied by line 110?
There was a problem hiding this comment.
if we have 2 aggregate types we return true
|
|
||
| // if allowCStyleInitList, process any ctor which comes next using the most members possible. ioArgIndex may not be 0. | ||
| // if !allowCStyleInitList, process any ctor that exactly matched our argument count. ioArgIndex must start at 0. | ||
| if (!allowCStyleInitList && ctorParamCount != Index(argCount)) |
There was a problem hiding this comment.
What if __init takes a variadic parameter pack?
There was a problem hiding this comment.
This requires a test-case
I believe coerce logic will function correctly with type packs (meaning type packs will work as intended), but I never thought-of/had-time to make a test.
| /// init returning data. Instead we will call `$ZeroInit` through this logic below. | ||
| Expr* _tryToSpecialCaseOverloadDefaultConstructWithoutInit(SemanticsVisitor* visitor, SemanticsVisitor::OverloadResolveContext& context, Expr* expr, OverloadCandidate* bestCandidate) | ||
| { | ||
| if (context.argCount == 0) |
There was a problem hiding this comment.
should we make sure the canddiate we are calling is a ctor decl?
what are we trying to do here? If we have a ctor candidate that takes more than 0 parameters but we are trying to call it with 0 args, we form an initializerList coercion instead? but wouldn't that coercion fail again due to lack of ctor? Why do we need to do this?
If we have struct S { __init (x) } and called with S s = {};, it should be an error?
There was a problem hiding this comment.
This was a workaround we kept from the previous implementation (but changed to be less of a hack). The main idea is that some structs may be treated as if they have an __init despite not ever declaring an __init
The goal is that we will zeroInit these objects
|
I think @kaizhangNV will have to take over and finish review of this PR (from what I understand), I apologize for the inconvenience this may cause. (I will try to answer questions if they come up though) |
| } | ||
|
|
||
| template<typename T> | ||
| bool _containsTargetType(ASTBuilder* m_astBuilder, NodeBase* target, HashSet<NodeBase*>& visitedNodes) |
There was a problem hiding this comment.
this is very heavy weight recursion, why do we need this?
There was a problem hiding this comment.
We need this or a cached version of some sort (I agree this is quite an expensive function) to detect if we need our ctor to be tagged with "CudaHost" due to CudaHost only type being found.
There was a problem hiding this comment.
Note: I agree, a legalization pass for 1 target is a better idea than this expensive recursive routine.
| } | ||
|
|
||
| // pre-calculate any requirements of a CudaHostAttribute | ||
| HashSet<VarDeclBase*> requiresCudaHostModifier; |
There was a problem hiding this comment.
It doesn't seem to worth this much complexity for such a corner case. We need to get rid of all the TorchTensorType` logic here, and just insert the cuda host decoration in a separate legalization pass when compiling to pytorch.
| } | ||
|
|
||
| if (structDeclInfo.defaultCtor) | ||
| // Compiler generated ctor may be destroyed |
There was a problem hiding this comment.
Why do we need to remove a ctor? Can we just not generate it in the first place?
There was a problem hiding this comment.
I think this ctor-deletion routine can just be removed completely, you are correct.
We should no longer be creating __init functions which need to be deleted in our bodyVisitor.
This must hold true since otherwise we would have a mismatch of conformances from the conformance AST-Pass and actual conformances (since deleting a ctor breaks conformances).
There was a problem hiding this comment.
After reading the code carefully, I found that we cannot avoid removing the ctor based on current change.
This change create a empty default ctor during SemanticsDeclConformancesVisitor::visitAggTypeDecl,
and it will fill the body of this default ctor in SemanticsDeclBodyVisitor::visitAggTypeDecl.
But there're some situations where it will not fill the body of the ctor, so if that is the case, we will remove the empty default ctor. (Though I still don't quite understand why we have to remove it, removing it will cause lots of errors).
We're actually able to detect those situations during creating the ctor, however the logic to check those situations are part of SemanticsDeclBodyVisitor::visitAggTypeDecl, and there is not a very good way to re-use the results of this logic.
So I'd say let's keep it, as cost of removing it could be smaller than "detecting and not create it".
| structDecl->invalidateMemberDictionary(); | ||
| structDecl->buildMemberDictionary(); | ||
| } | ||
| } | ||
|
|
||
| // Fill in our $ZeroInit | ||
| if (auto zeroInitListFunc = findZeroInitListFunc(structDecl)) |
There was a problem hiding this comment.
I think we shouldn't need this at all because our IR already knows how to zerzo init a thing.
We can simply do:
this = DefaultConstruct(This);
at the begining of the ctor, where DefaultConstruct(T) lowers to kIROp_DefaultConstruct(T)
There was a problem hiding this comment.
We could add a flag to DefaultConstructExpr to remove the need for $ZeroInit.
To elaborate, DefaultConstructExpr does not handle initExpr on a struct member (from what I remember).
We need a flag that we can set to tell Slang when lowering a DefaultConstructExpr that it should take into account initExpr of varDecl's.
|
@ArielG-NV there is no expectation that you will address any of these. |
|
I will leave a 👍 on tasks that should be addressed |
|
@ArielG-NV do you have a non-NV github account that we can reach you? |
|
My non-NV GitHub would be @16-Bit-Dog |
Fixes #3406
Fixes #4461
PR with documentation: #4988
Changes (split up into major files):
source/slang/diff.meta.slangTorchTensorType. This was added since Slang requires allTorchTensorobjects to have a CUDAHost attribute. When auto-generating constructors Slang needs to be aware of this type to avoid generating invalid code for CUDAHost variable using functions (automatically addCUDAHostifTorchTensoris found).source/slang/hlsl.meta.slang__init()was added toTextureFootprintto more correctly do what the struct requires (this also avoided at the time a weird compile failure that only the stdlib could cause).literal generics(Shape.dimensions) properly in our use case. We work around this by generating a simple copy of ourliteral genericand letting Slang optimize this value out to directly useShape.dimensions.source/slang/slang-ast-decl.hprivate,internal, andpublicgroups of members. If a Ctor is used forpublicandprivateandinternalscopes we need to store this information so that when we look forInheritanceDeclctor's, we know which "member-wise ctor" should be used given a visibility.ConstructorTagsallows us to store this information.source/slang/slang-check-conversion.cppUseLegacyLogicistrue).docs\user-guide\02-conventional-features.md)source/slang/slang-check-decl.cppSemanticsDeclConformancesVisitor.TorchTensortyped variable is found we auto-add a CUDA-host attribute (host only function)TreatAsDifferentiableExpr, we do not automatically error with "invalidno_diffuse-case", this is incorrect behavior. We only error ifTreatAsDifferentiableExpris of flavorno_diff.Other changes:
$ZeroInitto allow us 2 important functionalities: 1. Allows us to use{}inside a__init()2. it allows us to zero initialize with{}for when a user does not provide a__init()(which is expected behavior).tests\compute\type-legalize-global-with-init.slang.