shader-slang / slang

Making it easier to work with shaders
MIT License
2.07k stars 177 forks source link

Partial implementation of static_assert #4294

Closed jkwak-work closed 3 months ago

jkwak-work commented 3 months ago

Partially implements static_assert(C,M) where C is a compile-time constant value and M is an error message gets printed when C is evaluated to false.

Currently it can be used like a function call and it cannot be used as a part of struct declaration or in a global scope.

csyonghe commented 3 months ago

After validation, we need to remove the static assert instructions to allow dead code elim pass to clean up the code so we don't generate code for things that are only used in static assert.

jkwak-work commented 3 months ago

After validation, we need to remove the static assert instructions to allow dead code elim pass to clean up the code so we don't generate code for things that are only used in static assert.

The place I picked is right after the specialization is done; line 916. And I see that there is a call to eliminateDeadCode() later at line 1181.

    eliminateDeadCode(irModule, deadCodeEliminationOptions);

I am not sure what's the best way to make sure the IR is removed. I attempted a few different ways to check and it seems like removed but not sure. I will keep trying.

csyonghe commented 3 months ago

This is a good place to do the check. As long as all static_assert inst are removed we are fine. No need for other verifications.

csyonghe commented 3 months ago

If the inst is cleaned up after autodiff, we also need to make sure the autodiff pass handles the inst.

csyonghe commented 3 months ago

A case should be added to slang-ir-autodiff-fwd.cpp:1944 for the opcode to be treated as non differentiable.

jkwak-work commented 3 months ago

A case should be added to slang-ir-autodiff-fwd.cpp:1944 for the opcode to be treated as non differentiable.

Thanks for the information. I haven't had a chance to look at autodiff side code. I will check out this code and make sure it works well with autodiff.

csyonghe commented 3 months ago

Regarding the static_assert inst inside a function with a single __intrinsic_asm inst issue:

The reason here is that if a function has an intrinsic_asm, it will not be inlined, so the condition expr in the static_assert expr will never be substituted and folded into a constant. The key here is to allow the compiler to inline a function with `intrinsic_asm`. To do that, we need the following changes:

  1. Extend GenericAsm instruction to allow it to take additional arguments, so that GenericAsm("$0 $1", arg0, arg1) means that $0 is going to be emitted from arg0, $1 is going to be emitted from arg1.
  2. Lower __intrinsic_asm "str"; into GenericAsm("str", param0, param1, ...) where param0, param1 are the parameters of the function.
  3. Make GenericAsm not a terminator inst, and just a normal inst.

With these changes, we can inline a function with GenericAsm just like normal functions, and the operands to GenericAsm will be substituted from the callee arguments during inlining. Once a function with GenericAsm and StaticAssert insts are inlined, the checks for StaticAssert will get the folded value and will work as expected.

jkwak-work commented 3 months ago

It appears that StaticAssert IR is not eliminated yet and I am not sure if it can be eliminated.

The reason why it is marked as alive is because shouldInstBeLiveIfParentIsLive() and mightHaveSideEffects() returns true for it. When I changed mightHaveSideEffects() to return false, it gets eliminated, but then static_assert no longer works.

The problem is that when we compile stdlib, kIROp_StaticAssert shouldn't be removed and when we compile the user code, the static assertion check should happen.

I am not sure at what point we can remove kIROp_StaticAssert if it can be removed.

jkwak-work commented 3 months ago

Regarding the static_assert inst inside a function with a single __intrinsic_asm inst issue:

The reason here is that if a function has an intrinsic_asm, it will not be inlined, so the condition expr in the static_assert expr will never be substituted and folded into a constant. The key here is to allow the compiler to inline a function with `intrinsic_asm`. To do that, we need the following changes:

  1. Extend GenericAsm instruction to allow it to take additional arguments, so that GenericAsm("$0 $1", arg0, arg1) means that $0 is going to be emitted from arg0, $1 is going to be emitted from arg1.
  2. Lower __intrinsic_asm "str"; into GenericAsm("str", param0, param1, ...) where param0, param1 are the parameters of the function.
  3. Make GenericAsm not a terminator inst, and just a normal inst.

With these changes, we can inline a function with GenericAsm just like normal functions, and the operands to GenericAsm will be substituted from the callee arguments during inlining. Once a function with GenericAsm and StaticAssert insts are inlined, the checks for StaticAssert will get the folded value and will work as expected.

Thanks for this information. It sounds like a limitation more than a bug; or it can be seen as a known issue.

It doesn't seem like a trivial bug fix. And I will go with a workaround for the GLSL integer texture support task this time.

Do you think we need a new issue for this limitation? If yes, I can file one with a repro code.

csyonghe commented 3 months ago

The place you are invoking the pass is after the loop that calls specializeModule, so at this time we have specialized all generics that we can specialize, and it should be fine to remove all static assert insts. What problem are you seeing?

jkwak-work commented 3 months ago

The place you are invoking the pass is after the loop that calls specializeModule, so at this time we have specialized all generics that we can specialize, and it should be fine to remove all static assert insts. What problem are you seeing?

Your explanation makes sense. The problem I saw is that the test I added in this PR fails in a way that some of the static assertion failed as expected but some of them were silent as if they were removed.

I will need to debug and investigate it more.

jkwak-work commented 3 months ago

It turned out that I shouldn't have removed the instance while iterating the children.