[spirv] decouple argument scope from call-by-value decision by jaebaek · Pull Request #1991 · microsoft/DirectXShaderCompiler

jaebaek · 2019-03-01T20:32:18Z

Previously, when passing a variable whose scope is not Function,
DXC creates a temporal variable with Function scope and copies
the value of the variable to the temporal one. It produces legal
SPIR-V code but it causes unnecessary loads / stores if the
function is inlined by spirv-opt.

This commit decouples argument scope from call-by-value
decision to avoid creation/load/store of temporal variables.
This generates illegal SPIR-V code but spirv-opt HLSL
legalization handles it.

Related to #1977

msftclas · 2019-03-01T20:32:30Z

All CLA requirements met.

AppVeyorBot · 2019-03-01T21:17:32Z

✅ Build DirectXShaderCompiler 1.0.1446 completed (commit e098adcb17 by @jaebaek)

ehsannas

You also need to add a test for the case where we are calling a structure member function.

For example:

struct testStruct {
	int foo() {
        // ...
	}
};

groupshared testStruct sharedStruct;

void main() {
    int result = sharedStruct.foo();
}

I believe you haven't handled this case.

ehsannas · 2019-03-04T16:46:54Z

tools/clang/test/CodeGenSPIRV/cs.groupshared.function-param.hlsl

please run clang-format on this file.

AppVeyorBot · 2019-03-05T19:43:48Z

✅ Build DirectXShaderCompiler 1.0.1463 completed (commit 158c41535a by @jaebaek)

AppVeyorBot · 2019-04-15T20:26:22Z

✅ Build DirectXShaderCompiler 1.0.1722 completed (commit db750c0b3f by @jaebaek)

ehsannas · 2019-04-16T15:17:31Z

tools/clang/lib/SPIRV/SpirvEmitter.cpp

What about variables in the Private storage class? (e.g. static variable)

I believe with the new behavior of SPIRV-Tools, we should never create a temporary variable because of storage class mismatch. (we still do need to create a temporary variable if the param is an rvalue).
If you're not sure, you can check with @s-perron about what the unoptimized output of DXC should look like.

ehsannas · 2019-04-16T15:22:31Z

tools/clang/lib/SPIRV/SpirvEmitter.cpp

What about variables in the Private storage class? (e.g. static variable)

again, I think with the new SPIRV-Tools behavior, we shouldn't create temporary variables unless we have an rvalue? Also think about the cases where the function parameter is out or inout.

ehsannas · 2019-04-16T15:23:40Z

tools/clang/test/CodeGenSPIRV/cs.groupshared.function-param.hlsl

Also add tests for cases where the param is out or inout. e.g.

int bar(out int x) { ... }

int bar(inout int x) { ... }

ehsannas · 2019-04-16T15:24:05Z

tools/clang/test/CodeGenSPIRV/cs.groupshared.struct-function.hlsl

should also add tests for static cases (to get Private storage class).

AppVeyorBot · 2019-04-29T21:40:59Z

❌ Build DirectXShaderCompiler 1.0.1767 failed (commit 8b59984b7f by @jaebaek)

AppVeyorBot · 2019-04-30T20:22:56Z

❌ Build DirectXShaderCompiler 1.0.1777 failed (commit bd797ab890 by @jaebaek)

AppVeyorBot · 2019-05-13T19:27:51Z

❌ Build DirectXShaderCompiler 1.0.1862 failed (commit 2a879cfc34 by @jaebaek)

jaebaek · 2019-05-14T19:48:19Z

tools/clang/test/CodeGenSPIRV/type.append-structured-buffer.hlsl

This is removed because it is not generated with the newer SPIRV-Header/Tools

I just realized this is wrong. Let me update it.

AppVeyorBot · 2019-05-15T15:47:12Z

✅ Build DirectXShaderCompiler 1.0.1873 completed (commit ee0c23173b by @jaebaek)

AppVeyorBot · 2019-05-22T15:35:18Z

✅ Build DirectXShaderCompiler 1.0.1902 completed (commit 0ec256cdcd by @jaebaek)

ehsannas · 2019-05-16T21:04:31Z

tools/clang/lib/SPIRV/SpirvEmitter.cpp

ehsannas · 2019-05-16T21:12:59Z

tools/clang/lib/SPIRV/SpirvEmitter.cpp

s/temporal/temporary

ehsannas · 2019-05-16T21:13:40Z

tools/clang/lib/SPIRV/SpirvEmitter.cpp

s/must be always Function scope/must always be in Function scope

ehsannas · 2019-05-16T21:22:12Z

tools/clang/lib/SPIRV/SpirvEmitter.cpp

Better comment:
If it calls a non-static member function, the object itself is argument 0, and therefore all other argument positions are shifted by 1.

ehsannas · 2019-05-22T19:29:03Z

tools/clang/test/CodeGenSPIRV/decoration.no-contraction.hlsl

oops! Thanks for fixing this! :)

ehsannas · 2019-05-22T19:52:25Z

tools/clang/test/CodeGenSPIRV/oo.struct.method.hlsl

So much nicer now! :)

ehsannas · 2019-05-22T20:11:42Z

tools/clang/lib/SPIRV/SpirvEmitter.cpp

maybe you need to add a beforeHlslLegalization parameter to spirvToolsValidate

ehsannas · 2019-05-22T20:12:10Z

tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp

this one is using setRelaxLogicalPointer instead of setBeforeHLSLLegalization. why?

This test does not contain implicit type conversion between isomorphic types. It only uses a illegal storage class i.e., GroupShared instead of function. In this case, enabling only --relax-logical-pointer is fine.

I think it would be best to use before-hlsl-legalization.

ehsannas · 2019-05-22T20:12:25Z

tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp

these ones can maybe still keep using setRelaxLogicalPointer ?

All of them must be setRelaxLogicalPointer because they have isomorphic function arguments that are actually different from function parameters.

AppVeyorBot · 2019-05-23T20:52:41Z

✅ Build DirectXShaderCompiler 1.0.1907 completed (commit 9ff3c76a1f by @jaebaek)

ehsannas

As discussed offline, we should remove relaxLogicalPointer from the backend, and only use beforeHlslLegalization. You can do it in a follow-up CL. Thanks

This issue is related to HLSL to SPIR-V compile. Previously, when passing a variable with Workgroup scope, DXC creates a temporal variable with Function scope and copies the Workgroup variable to the temporal one. This prevents Workgroup variables from being a function parameter. However, it causes unnecessary loads / stores if the function is inlined by spirv-opt. If a function param is a Workgroup variable, this commit does not create a temporal one. This is illegal but spirv-opt legalization will handle it. Related to microsoft#1977

…-legalization

AppVeyorBot · 2019-05-24T20:12:37Z

✅ Build DirectXShaderCompiler 1.0.1912 completed (commit d924eb080a by @jaebaek)

jaebaek requested a review from ehsannas March 1, 2019 20:32

ehsannas assigned jaebaek Mar 1, 2019

ehsannas added the spirv Work related to SPIR-V label Mar 1, 2019

ehsannas changed the title ~~Not create temp var for groupshared function param~~ [spirv] Not create temp var for groupshared function param Mar 4, 2019

ehsannas suggested changes Mar 4, 2019

View reviewed changes

jaebaek force-pushed the groupshared_function_param branch from f214c31 to a49bf6e Compare March 5, 2019 17:14

ehsannas mentioned this pull request Mar 7, 2019

Legalization of Workgroup function parameter KhronosGroup/SPIRV-Tools#2430

Closed

jaebaek force-pushed the groupshared_function_param branch from d6bbccb to 541c834 Compare April 15, 2019 19:38

ehsannas suggested changes Apr 16, 2019

View reviewed changes

jaebaek force-pushed the groupshared_function_param branch from 541c834 to 1ba5a80 Compare April 29, 2019 20:40

jaebaek force-pushed the groupshared_function_param branch from 626526f to 8aa3f16 Compare May 13, 2019 19:20

jaebaek commented May 14, 2019

View reviewed changes

jaebaek force-pushed the groupshared_function_param branch from 8aa3f16 to 5dc3b6a Compare May 15, 2019 14:43

jaebaek changed the title ~~[spirv] Not create temp var for groupshared function param~~ [spirv] decouple argument scope from call-by-value decision May 15, 2019

jaebaek force-pushed the groupshared_function_param branch from 5dc3b6a to 4abcbcb Compare May 22, 2019 13:59

ehsannas reviewed May 22, 2019

View reviewed changes

ehsannas approved these changes May 24, 2019

View reviewed changes

jaebaek added 3 commits May 24, 2019 15:01

Handle calling struct member function

245e058

clang-format

8348237

jaebaek added 5 commits May 24, 2019 15:01

fix

e34e96e

type isomorphic

15f6187

fix

45745d0

Replace --relax-logical-pointer for function param with --before-hlsl…

7c5a694

…-legalization

fix

518a346

jaebaek force-pushed the groupshared_function_param branch from ee96997 to 518a346 Compare May 24, 2019 19:08

fix

bf27ffc

jaebaek merged commit c9b258a into microsoft:master May 27, 2019

jaebaek deleted the groupshared_function_param branch May 27, 2019 21:11

jaebaek mentioned this pull request May 27, 2019

[spirv] Suboptimal code generation for groupshared struct member functions/functions accepting groupshared arrays #1977

Closed

Conversation

jaebaek commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AppVeyorBot commented Mar 1, 2019

Uh oh!

ehsannas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Mar 5, 2019

Uh oh!

AppVeyorBot commented Apr 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Apr 29, 2019

Uh oh!

AppVeyorBot commented Apr 30, 2019

Uh oh!

AppVeyorBot commented May 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented May 15, 2019

Uh oh!

AppVeyorBot commented May 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jaebaek commented Mar 1, 2019 •

edited

Loading

msftclas commented Mar 1, 2019 •

edited

Loading

ehsannas left a comment •

edited

Loading