Member-wise constructor generation with visibility control #5107

kaizhangNV · 2024-09-18T17:40:14Z

Rebased version of PR #4854.

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

1. fix breaking tests which cannot be fixed by adding 'old style slang array init-list syntax' support, specifically for constructing a struct without an explicit '{}' 2. clean up tests

1. resolve generics which are associated to a struct instance during init list evaluation 2. fix autodiff test with incorrect init-list

…mitations.

…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.

…for ctor

…ness bug Adding 2 things: 1. add no_diff to synth'ed ctor if param is no_diff 2. Fix bug where DifferentialType 'ownedScope' is not setup to track the associated contained (causes 'ThisExpr' to resolve incorrectly)

…zedMemberwiseCtor'

…t' to '_tryOverloadWithDefaultInit' This function just tried to resolve a problem that struct S {int x}; S s = S(); We will try to overload `S()` with synthesized default initializer. Because there is no such function with signature of "S S()".

csyonghe · 2024-09-20T21:45:36Z

source/slang/slang-check-conversion.cpp


-                    if( coercedArg )
+                    // 2. We have a `{}` (0 arguments), try to coerce


I think we should delete all these logic here from 535 till 671, and reuse the existing ResolveInvoke logic to resolve the constructor call.

Once we reach here, we are about to turn initializer list {...} into a ctor call that is StructType(...).
This can be done by:

create a AST node that is InvokeExpr(calleeExpr, coercedArgs), where:
calleeExpr is a VarExpr(declRef = toStructDeclRef, type = TypeType(toType), checked = true).

Create a clone of the current semantic visitor to try to check the invoke Expr in a sub context so that any checking errors will not leak into the current visitor:

DiagnosticSink tempSink(getSourceManager(), nullptr); ExprLocalScope localScope; SemanticsVisitor subVisitor(withSink(&tempSink).withParentFunc(synFuncDecl).withExprLocalScope(&localScope));

call subVisitor.ResolveInvoke() on the invokeExpr. If tempSink doesn't have any errors, then we succeeded in calling the ctor, and the result of ResolveInvoke is the *outToExpr we need to return, otherwise, we return false. We may want to copy out any error message here, and report them to the user if the legacy logic also failed at line 780.

This should handle all the generic case or the variadic case just fine using the same logic in ResolveInvoke.

Create a sub-task to address this issue: #5134.
Note, I won't submit a new PR for this. It just that I found out the original task took much longer than we estimated (estimation is just 1 day! And I already spent 3 days). And it could take longer, so split the task smaller so that I will have a clear goal what I'm going to resolve.

csyonghe · 2024-09-20T21:55:37Z

source/slang/slang-check-conversion.cpp

@@ -201,11 +221,24 @@ namespace Slang
        return baseStructDeclRef;
    }

+    GenericAppDeclRef* _getGenericAppDeclRefType(Type* argType)
+    {
+        auto argDeclRefType = as<DeclRefType>(argType);


auto declRef = isDeclRefTypeOf<Decl>(argType); if (!declRef) return nullptr; return as<GenericAppDeclRef>(declRef.getDeclRefBase());

csyonghe · 2024-09-20T21:59:56Z

source/slang/slang-check-conversion.cpp

-        // is possible (a type conversion exists).
-        //
-        return canCoerce(toType, fromExpr->type, fromExpr);
+        // If 2 types are "broadly equal" we know the types can be coerced directly


I want to understand why this change here, along with the change in line 151 is needed.

I think it should be fine if we revert all changes to this function and in 151.

If things break, I want to understand why.

csyonghe · 2024-09-20T22:12:36Z

source/slang/slang-check-decl.cpp

+        // pre-calculate any requirements of a CudaHostAttribute
+        HashSet<VarDeclBase*> requiresCudaHostModifier;
+        for (auto member : membersOfStructDeclInstance)
+            if (containsTargetType<TorchTensorType>(m_astBuilder, member.getDecl()->type.type))


This is going to be bad for performance because it revisits the entire type hierarchy for all the member types.

An alternative is to add the required CudaHost attribute in an IR pass to make things cleaner.

Create sub-task #5136 to address this.

csyonghe · 2024-09-20T22:13:02Z

source/slang/slang-check-decl.cpp

        }

-        if (structDeclInfo.defaultCtor)
+        // Note: we assume only 1 inheritance decl currently.


There is a lot of code added to this checkAggType method. We really need to split them into separate functions for readability.

Create sub-task #5135 to address comment.

csyonghe · 2024-09-20T22:17:59Z

source/slang/slang-check-decl.cpp

+            seqStmt->stmts.insert(ctorInfo.m_insertOffset++, seqStmtChild);
+        }
+
+        // Compiler generated ctor may be destroyed


What will go wrong if we just leave the empty ctor there without deleting it?

I just figure out why Ariel wants to delete the empty default ctor.
Considering this case

struct S { int a; } S s;

In this case, based on currently logic, we will synthesize a empty default ctor: S.$init(This), and for S s;, we will call this default ctor to construct the object. So this will cause an issue that slang will report a warning that a is not initialized.
While, if there is no default ctor (removing the default ctor), the default ctor won't be called to construct s, no warning will be reported.

I didn't participant in your previous discussion about what the expected behavior it is. I know that for C++, the default ctor will be called, and C++ won't report warning for that.

If we still want to insist this behavior, I think the warning reported here is actually helppful because it kind of reminds developers to initialize it or use s = {} to zero init it.

BTW, there is actually noting wrong that if we don't delete this ctor. But I need to fix bunch of the tests code.

@csyonghe to comment this reply first.

It is worth noting that there was a second reason that makes this change (in isolation) slightly problematic.

By auto generating init() functions, we will be changing how code-gen is structured since certain previously uninitialized struct values will now have an assignment to a function (even though this init() effectively does nothing).

Example of an init() function that effectively does nothing if user writes RayDesc ray; (assuming we don't delete init() anymore)

#line 15702 "hlsl.meta.slang" RayDesc RayDesc_x24init_0() { #line 15702 RayDesc _S1; #line 15702 return _S1; }

This means that the following changes can be observed:

Certain tests will need to be adjusted due to making incorrect assumptions on how code-gen works with uninitialized values.

Code-gen will include init() function's that effectively do nothing.

create sub-task to remove this hack: Create a sub-task to remove this hack: #5138

csyonghe · 2024-09-20T22:23:07Z

source/slang/slang-check-decl.cpp

+            auto defaultConstructExpr = createDefaultConstructExprForType(m_astBuilder, structDeclType, seqStmt->loc);
+
+            // Part 1
+            // Assign DefaultConstructExpr to `this`


I think we only need to execute part 1 if "-zero-init" option is set.

csyonghe · 2024-09-20T22:29:45Z

source/slang/slang-check-overload.cpp

+    /// init returning data. Instead we will call `$DefaultInit` through this logic below.
+    ///     struct S { int x; }
+    ///     S s = S(); // This will call S.$DefaultInit().
+    Expr* _tryOverloadWithDefaultInit(SemanticsVisitor* visitor, SemanticsVisitor::OverloadResolveContext& context, Expr* expr, OverloadCandidate* bestCandidate)


Can you explain this again? I still don't understand what it is trying to do. For the example shown in the comment here, haven't we already synthesized the default ctors for S, so that the overload resolution will resolve into a call to that defualt ctor already? Why do we need to run the coercion logic again here?

This is code that mostly carries over from how TOT did things. The quick answer is that "we need this function to handle an edge case related to how Slang always assigns default-constructors to uninitialized values if present".

I will break down why we have this function into 2 parts below.

What _tryOverloadWithDefaultInit does:

If I type the following code, _tryOverloadWithDefaultInit will be used to resolve MyStruct():

struct MyStruct { float a; }; [numthreads(1,1,1)] void computeMain(int3 tid : SV_DispatchThreadID) { MyStruct val = MyStruct(); // effectively `MyStruct val = {};` }

Why do we have this function:

[using the code above] We have 2 requirements for any call to MyStruct() or related (Ex: RayDesc()):

We need to treat MyStruct() as a 'default-initialize'. This is a requirement since lots of code relies on this behavior.

We cannot treat MyStruct() as a 'default-initialize' using a compiler generated init() since this will force all uninitialized struct types into calling their init() ('default-initialize'). This is undesired behavior.

An example where rule 2 is important:

struct MyStruct { float a; }; [numthreads(1,1,1)] void computeMain(int3 tid : SV_DispatchThreadID) { MyStruct val1 = MyStruct(); // effectively `MyStruct val = {};`. This variable is default initialized. MyStruct val2; // Effectively `MyStruct val2;`. This variable is uninitialized. }

Create a sub-task to remove this hack: #5138

csyonghe

Looks good overall, but I think there are few important cleanups we need to do before checking this in.

This commit will be reverted when the initializer list re-work is done. This is just to unblock the test failure when we're in the middle ground.

CLAassistant · 2024-11-25T19:26:04Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ kaizhangNV
❌ ArielG-NV
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

ArielG-NV added 30 commits August 15, 2024 11:26

Merge branch 'master' into initializer-list-visibility

ce554c3

fix bug which causes crash

ceb70c4

Merge branch 'initializer-list-visibility' of https://github.com/Arie…

6df637d

…lG-NV/slang into initializer-list-visibility

git merge fix

0110058

Merge branch 'master' into initializer-list-visibility

a1b1c50

fix default init expr with init expr paramDecl

0dca495

Merge branch 'initializer-list-visibility' of https://github.com/Arie…

44de3d4

…lG-NV/slang into initializer-list-visibility

Merge branch 'master' into initializer-list-visibility

bab4c9f

Fixes

d82d68b

1. fix breaking tests which cannot be fixed by adding 'old style slang array init-list syntax' support, specifically for constructing a struct without an explicit '{}' 2. clean up tests

fix bug and incorrect test

892016a

1. resolve generics which are associated to a struct instance during init list evaluation 2. fix autodiff test with incorrect init-list

fix more tests with invalid init-lists

5755c0a

fix swizzle causing a failiure with checkExpr to resolve an Invoke

f6d5bd3

Merge branch 'master' into initializer-list-visibility

251314e

init expr with default ctor is now more restrictive due to current li…

045e27a

…mitations.

Merge branch 'initializer-list-visibility' of https://github.com/Arie…

6d50283

…lG-NV/slang into initializer-list-visibility

Revert "init expr with default ctor is now more restrictive due to cu…

4c36d3a

…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.

undo some code removal

f8bf220

make a few additions/changes to init list logic:

1e7819a

1. allow a more formalized init-list logic for flattened init-lists's 2. fixing invalid tests

remove unused var

1e4ec35

cleanup some tests, logic and auto-documentation code

95e719d

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.

add missing include (clang/gcc)

04f7177

Merge branch 'master' into initializer-list-visibility

b3d18c7

Clean up invalid tests, fix some bugs, add better use-site inference …

67c9d29

…for ctor

clang/gcc compiler warning

7476962

cyclic type solution

78feed6

include header that defines another function (whom is in a header).

98cd3b3

fix bit fields

a788a53

kaizhangNV force-pushed the arial/initializer-list branch from 9d28326 to 1439309 Compare September 18, 2024 18:32

Merge branch 'master' into arial/initializer-list

2682128

kaizhangNV force-pushed the arial/initializer-list branch from 1439309 to 2682128 Compare September 18, 2024 18:36

kaizhangNV added 2 commits September 19, 2024 12:12

Merge branch 'master' into tmp

bb22fa2

fix easy comments

5401494

kaizhangNV force-pushed the arial/initializer-list branch from 9333fe1 to 5401494 Compare September 19, 2024 19:58

refactor _getDefaultCtor

39268d6

kaizhangNV force-pushed the arial/initializer-list branch from 203c628 to 39268d6 Compare September 19, 2024 21:04

kaizhangNV added 5 commits September 19, 2024 19:19

change function '_doesCtorExpectInitializerListUsage' to '_isSynthisi…

5771007

…zedMemberwiseCtor'

Merge branch 'master' into arial/initializer-list

89d221c

replace all 'ZeroInit' to 'DefaultInit'

d966b3f

remove the WAR in hlsl.meta.slang

69cb426

kaizhangNV marked this pull request as ready for review September 20, 2024 20:46

csyonghe reviewed Sep 20, 2024

View reviewed changes

kaizhangNV added 2 commits September 23, 2024 20:47

Do not remove the default ctor

1861df4

DNS: Modify all the tests that does not have empty initializer

fde9141

This commit will be reverted when the initializer list re-work is done. This is just to unblock the test failure when we're in the middle ground.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Member-wise constructor generation with visibility control #5107

Member-wise constructor generation with visibility control #5107

kaizhangNV commented Sep 18, 2024

csyonghe Sep 20, 2024 •

edited

Loading

kaizhangNV Sep 23, 2024

csyonghe Sep 20, 2024

csyonghe Sep 20, 2024

csyonghe Sep 20, 2024

csyonghe Sep 20, 2024

kaizhangNV Sep 23, 2024

csyonghe Sep 20, 2024

kaizhangNV Sep 23, 2024

csyonghe Sep 20, 2024

kaizhangNV Sep 23, 2024 •

edited

Loading

kaizhangNV Sep 23, 2024

16-Bit-Dog Sep 23, 2024 •

edited

Loading

kaizhangNV Sep 23, 2024

csyonghe Sep 20, 2024

csyonghe Sep 20, 2024

16-Bit-Dog Sep 23, 2024 •

edited

Loading

kaizhangNV Sep 23, 2024

csyonghe left a comment

CLAassistant commented Nov 25, 2024 •

edited

Loading


		if( coercedArg )
		// 2. We have a `{}` (0 arguments), try to coerce

Member-wise constructor generation with visibility control #5107

Are you sure you want to change the base?

Member-wise constructor generation with visibility control #5107

Conversation

kaizhangNV commented Sep 18, 2024

csyonghe Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaizhangNV Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

16-Bit-Dog Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

16-Bit-Dog Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csyonghe left a comment

Choose a reason for hiding this comment

CLAassistant commented Nov 25, 2024 • edited Loading

csyonghe Sep 20, 2024 •

edited

Loading

kaizhangNV Sep 23, 2024 •

edited

Loading

16-Bit-Dog Sep 23, 2024 •

edited

Loading

16-Bit-Dog Sep 23, 2024 •

edited

Loading

CLAassistant commented Nov 25, 2024 •

edited

Loading