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: 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
|
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.
|
@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.