Skip to content

Handle OpKill better#2933

Merged
s-perron merged 2 commits intoKhronosGroup:masterfrom
s-perron:opkill
Oct 4, 2019
Merged

Handle OpKill better#2933
s-perron merged 2 commits intoKhronosGroup:masterfrom
s-perron:opkill

Conversation

@s-perron
Copy link
Copy Markdown
Collaborator

@s-perron s-perron commented Oct 3, 2019

We want to handle OpKill better. The wrap opkill causes lots of extra
code to be generated, even when they are not needed to avoid the main
problem: OpKill cannot be found directly in a continue construct.

This change will be more selective on which functions the OpKill will be
wrapped and inlining will avoid inlining.

Fixes #2912

@s-perron
Handle OpKill better
4db054b 
We want to handle OpKill better.  The wrap opkill causes lots of extra
code to be generated, even when they are not needed to avoid the main
problem: OpKill cannot be found directly in a continue construct.

This change will be more selective on which functions the OpKill will be
wrapped and inlining will avoid inlining.

Fixes KhronosGroup#2912
@s-perron s-perron requested a review from ehsannas October 3, 2019 14:42
@s-perron s-perron self-assigned this Oct 3, 2019
@s-perron
Copy link
Copy Markdown
Collaborator Author

s-perron commented Oct 3, 2019

@greg-lunarg FYI

@paulthomson
Copy link
Copy Markdown
Contributor

paulthomson commented Oct 4, 2019

@s-perron Could a function call get moved into a continue construct by an optimization pass?

@s-perron
Copy link
Copy Markdown
Collaborator Author

s-perron commented Oct 4, 2019

@paulthomson I cannot think of any optimizations we currently do that would do that. The only one that would do anything similar to that would be code-sinking, but that should not be moving function calls.

ehsannas
ehsannas approved these changes Oct 4, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a couple of nits

source/opt/inline_pass.cpp Outdated

if (has_opkill) {
return false;
}
Copy link
Copy Markdown
Contributor

@ehsannas ehsannas Oct 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a nit; the code would be easier to understand with fewer nesting:

bool func_has_opkill = !func->WhileEachInst([](Instruction* inst) { return inst->opcode() != SpvOpKill; });
bool func_is_called_from_continue = funcs_called_from_continue_.count(func->result_id()) != 0;
if (func_has_opkill && func_is_called_from_continue)
  return false;

Copy link
Copy Markdown
Collaborator Author

@s-perron s-perron Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to do this exactly because I want to avoid walking the instruction of the function when possible. I'll find another way to clean it up.

ehsannas
ehsannas approved these changes Oct 4, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@s-perron s-perron merged commit c18c9ff into KhronosGroup:master Oct 4, 2019
@s-perron s-perron deleted the opkill branch October 4, 2019 17:05
dneto0 pushed a commit to dneto0/SPIRV-Tools that referenced this pull request Sep 14, 2024
@zoddicus
Rolling 5 dependencies (KhronosGroup#836)
05b2857 
Roll third_party/glslang/ 7bc0473..4b97a11 (5 commits)

KhronosGroup/glslang@7bc0473...4b97a11

$ git log 7bc0473..4b97a11 --date=short --no-merges --format='%ad %ae %s'
2019-10-06 dj2 single line
2019-10-03 dj2 Update appveyor and travis files
2019-10-03 dj2 Move install directory for SPIRV/ folder.
2019-09-29 cepheus HLSL: Fix KhronosGroup#1912: add attribute syntax for nonreadable/nonwritable
2019-09-27 cepheus HLSL: Fix KhronosGroup#1912: add attribute syntax for overriding image formats.

Roll third_party/googletest/ dc1ca9ae4..cd17fa2ab (8 commits)

google/googletest@dc1ca9a...cd17fa2

$ git log dc1ca9ae4..cd17fa2ab --date=short --no-merges --format='%ad %ae %s'
2019-10-05 soap Add documentation for pkg-config in cross-compilation settings
2019-10-05 soap Revert "Use pcfiledir for prefix in pkgconfig file"
2019-10-03 misterg Googletest export
2019-10-03 absl-team Googletest export
2019-10-02 absl-team Googletest export
2019-09-29 misterg Googletest export
2019-10-01 ant35rookie Fix typo in documents
2019-08-16 pbarker Add many missing override keywords

Roll third_party/re2/ 5bd613749..266119da0 (2 commits)

google/re2@5bd6137...266119d

$ git log 5bd613749..266119da0 --date=short --no-merges --format='%ad %ae %s'
2019-10-09 junyer Tidy up the ersatz benchmark library.
2019-10-07 junyer Move pod_array.h and sparse_{array,set}.h from util/ to re2/.

Roll third_party/spirv-headers/ 842ec90..b252a50 (1 commit)

KhronosGroup/SPIRV-Headers@842ec90...b252a50

$ git log 842ec90..b252a50 --date=short --no-merges --format='%ad %ae %s'
2019-09-24 lukasz.gotszald add cmake option SPIRV_HEADERS_SKIP_INSTALL

Roll third_party/spirv-tools/ 9eb1c9a..df15a4a (15 commits)

KhronosGroup/SPIRV-Tools@9eb1c9a...df15a4a

$ git log 9eb1c9a..df15a4a --date=short --no-merges --format='%ad %ae %s'
2019-10-09 cwallez CMake: Add support for building with emscripten (KhronosGroup#2948)
2019-10-09 stevenperron Update CHANGES
2019-10-08 stevenperron Link cfg and dominator analysis in the context (KhronosGroup#2946)
2019-10-08 afdx spirv-fuzz: add transformation and pass to construct composites (KhronosGroup#2941)
2019-10-08 paulthomson reduce: improve remove unref instr pass (KhronosGroup#2945)
2019-10-08 afdx spirv-fuzz: add disabled test to document known issue (KhronosGroup#2942)
2019-10-08 afdx spirv-fuzz: Add fuzzer pass to change selection controls (KhronosGroup#2944)
2019-10-07 jeremy-lunarg Enable OpTypeCooperativeMatrix specialization (KhronosGroup#2927)
2019-10-04 stevenperron Handle OpKill better (KhronosGroup#2933)
2019-10-04 greg Generate null pointer by converting uint64 zero to pointer. (KhronosGroup#2935)
2019-10-03 afdx spirv-fuzz: option to convert shader into a form that renders red (KhronosGroup#2934)
2019-10-03 32110296+AaronHaganAMD Add SPV_KHR_shader_clock validation (KhronosGroup#2879)
2019-10-03 paulthomson reduce/fuzz: improve command line args (KhronosGroup#2932)
2019-10-02 alanbaker Validate physical storage buffer restrictions (KhronosGroup#2930)
2019-10-01 paulthomson fuzz: add shrinker-temp-file-prefix (KhronosGroup#2928)

Created with:
  roll-dep third_party/effcee third_party/glslang third_party/googletest third_party/re2 third_party/spirv-cross third_party/spirv-headers third_party/spirv-tools
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ehsannas ehsannas ehsannas approved these changes

Assignees

@s-perron s-perron

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

#2790, #2842 OpKill changes causing size regressions

3 participants

@s-perron @paulthomson @ehsannas