Instrument: Add support for Buffer Device Address extension by greg-lunarg · Pull Request #2792 · KhronosGroup/SPIRV-Tools

greg-lunarg · 2019-08-13T19:36:54Z

Adds new pass to instrument shaders for GPU-assisted validation of shaders referencing through
physical storage buffer address. This pass is only intended to be called from the validation layer.
A command line option is added for developer convenience and is not documented.

greg-lunarg · 2019-08-14T15:55:07Z

@s-perron The failures don't look related to my changes. Are you comfortable going ahead with the review?

s-perron · 2019-08-14T16:53:55Z

I already started reviewing it. Later, Steven Perron

…

On Wed, Aug 14, 2019 at 11:55 AM greg-lunarg ***@***.***> wrote: @s-perron <https://github.com/s-perron> The failures don't look related to my changes. Are you comfortable going ahead with the review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2792?email_source=notifications&email_token=AHRTCJR7NK5LVNDN6KGW6MDQEQTGBA5CNFSM4ILOCPJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4JIDLQ#issuecomment-521306542>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHRTCJUB4M64DIOZOY5UJZ3QEQTGBANCNFSM4ILOCPJA> .

s-perron

Just some minor comment or naming changes.

s-perron · 2019-08-14T15:59:14Z

source/opt/inst_buff_addr_check_pass.cpp

@@ -0,0 +1,429 @@
+// Copyright (c) 2018 The Khronos Group Inc.
+// Copyright (c) 2018 Valve Corporation
+// Copyright (c) 2018 LunarG Inc.


The year is 2019.

Time sure flies ;)

s-perron · 2019-08-14T15:59:32Z

source/opt/inst_buff_addr_check_pass.h

@@ -0,0 +1,132 @@
+// Copyright (c) 2018 The Khronos Group Inc.
+// Copyright (c) 2018 Valve Corporation
+// Copyright (c) 2018 LunarG Inc.


The year should be 2019.

s-perron · 2019-08-14T16:00:06Z

test/opt/inst_buff_addr_check_test.cpp

@@ -0,0 +1,620 @@
+// Copyright (c) 2017 Valve Corporation
+// Copyright (c) 2017 LunarG Inc.


s-perron · 2019-08-14T16:59:22Z

source/opt/inst_buff_addr_check_pass.cpp

+  return new_ref_id;
+}
+
+bool InstBuffAddrCheckPass::AnalyzeBuffAddrReference(Instruction* ref_inst) {


This function could have a different name that reflects what it is checking. Something like IsPhysicalBuffAddrReference might read better.

s-perron · 2019-08-14T17:06:14Z

source/opt/inst_buff_addr_check_pass.cpp

+      {error_id, lo_uptr_inst->result_id(), hi_uptr_inst->result_id()},
+      &builder);
+  // Remember last invalid block id
+  uint32_t last_invalid_blk_id = new_blk_ptr->GetLabelInst()->result_id();


Will last_invalid_blk_id be different than invalid_blk_id?

Doesn't look like it. Must be an artifact from a previous incarnation of the algorithm.

s-perron · 2019-08-14T17:19:35Z

source/opt/inst_buff_addr_check_pass.h

+  // Generate code into |builder| to do search of the BDA debug input buffer
+  // for the buffer used by |ref_inst| and test that all bytes of reference
+  // are within the buffer. Returns id of boolean value which is true if
+  // search and test is successful, false otherwise


Missing period.

s-perron · 2019-08-14T17:24:32Z

source/opt/inst_buff_addr_check_pass.h

+      UptrVectorIterator<BasicBlock> ref_block_itr, uint32_t stage_idx,
+      std::vector<std::unique_ptr<BasicBlock>>* new_blocks);
+
+  // Analyze reference |ref_inst| and save components into |ref|.


What is |ref|? Looking at the implementation I do not see any saving going on.

Cut and paste error.

source/opt/inst_buff_addr_check_pass.h

s-perron · 2019-08-14T17:27:32Z

source/opt/inst_buff_addr_check_pass.h

+  // Generate instrumentation code for boolean test result |check_id|,
+  // adding new blocks to |new_blocks|. Generate conditional branch to a valid
+  // or invalid branch. Generate valid block which does original reference
+  // |ref_inst|. Generate invalid block which writes debug error output


The term "invalid block" is used in a number of places, but it is a bit confusing. The adjective makes it seem like the block itself it invalid. In this case, it might be better to refer to it as the "block that handles invalid references" or something like that.

I changed these to "valid reference block" and "invalid reference block" so it is more clear that it is the reference that is valid or invalid, and not the block.

s-perron · 2019-08-14T17:37:18Z

The ndk build is failing because you did not update Andriod.mk. You also need to update the BUILD.gn file.

greg-lunarg · 2019-08-14T17:53:27Z

The ndk build is failing because of the you did not update Andriod.mk. You also need to update the BUILD.gn file

Doh!

greg-lunarg · 2019-08-15T15:48:04Z

@s-perron I believe I have now addressed all concerns to date. Let me know if you have any others.

Roll third_party/glslang/ 37fc4d2..95609e6 (1 commit) KhronosGroup/glslang@37fc4d2...95609e6 $ git log 37fc4d2..95609e6 --date=short --no-merges --format='%ad %ae %s' 2019-08-14 johnkslang Set theme jekyll-theme-merlot Roll third_party/googletest/ 90a443f9c..c9ccac7cb (18 commits) google/googletest@90a443f...c9ccac7 $ git log 90a443f9c..c9ccac7cb --date=short --no-merges --format='%ad %ae %s' 2019-08-19 misterg Googletest export 2019-08-16 absl-team Googletest export 2019-08-16 absl-team Googletest export 2019-08-16 misterg Googletest export 2019-08-16 misterg Googletest export 2019-08-16 misterg Googletest export 2019-08-15 misterg Googletest export 2019-08-15 absl-team Googletest export 2019-08-12 absl-team Googletest export 2019-08-09 absl-team Googletest export 2019-08-09 absl-team Googletest export 2019-08-13 krystian.kuzniarek remove custom implementations of std::is_same 2019-08-14 krystian.kuzniarek remove a custom implementation of std::is_reference 2019-08-11 adam.f.badura Use -Wa,-mbig-obj for Cygwin/MinGW always 2019-08-11 krystian.kuzniarek remove an outdated comment 2019-08-08 krystian.kuzniarek remove a dead metafunction 2019-08-07 contact Update Bazel on Windows 2019-08-07 contact Prepare for Bazel incompatible changes Roll third_party/re2/ 67bce690d..be0e1305d (15 commits) google/re2@67bce69...be0e130 $ git log 67bce690d..be0e1305d --date=short --no-merges --format='%ad %ae %s' 2019-08-19 junyer Add Clang 9 to the Travis CI matrix. 2019-08-19 junyer Don't assume that iterators are just pointers. 2019-08-18 junyer No, it was right before. Try the /cygdrive form. 2019-08-18 junyer Try under 'C:\Program Files (x86)' instead. Sigh. 2019-08-18 junyer Ensure that CMake is in the path on Windows. 2019-08-18 junyer Comment on why we pin to Visual Studio 2015. 2019-08-18 junyer Attempt to avoid VCVARSALL.BAT breakage entirely. 2019-08-18 junyer Attempt to address VCVARSALL.BAT breakage. Sigh. 2019-08-18 junyer Argh. Try a different flag. 2019-08-18 junyer Try to upgrade Bazel harder on Windows. 2019-08-18 junyer Upgrade Bazel before trying to build with it. 2019-08-18 junyer Switch to Starlark for C++ rules. 2019-08-15 junyer Configure Kokoro to run CMake builds on Ubuntu. 2019-08-15 junyer Configure CMake to require version 3.5.1, which is what Xenial has. 2019-08-15 junyer Upgrade Travis CI from Trusty to Xenial. Roll third_party/spirv-tools/ f701237..bc62722 (8 commits) KhronosGroup/SPIRV-Tools@f701237...bc62722 $ git log f701237..bc62722 --date=short --no-merges --format='%ad %ae %s' 2019-08-18 stevenperron Handle overflow in wrap-opkill (KhronosGroup#2801) 2019-08-16 stevenperron More handle overflow in sroa (KhronosGroup#2800) 2019-08-16 greg Instrument: Add support for Buffer Device Address extension (KhronosGroup#2792) 2019-08-15 toomas.remmelg Update remquo validation to match the OpenCL Extended Instruction Set Specification (KhronosGroup#2791) 2019-08-15 jaebaek Use ascii code based characters (KhronosGroup#2796) 2019-08-14 jaebaek Change the way to include header (KhronosGroup#2795) 2019-08-14 alanbaker Fix validation of constant matrices (KhronosGroup#2794) 2019-08-14 stevenperron Replace OpKill With function call. (KhronosGroup#2790) 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

greg-lunarg force-pushed the inst53 branch 2 times, most recently from cd17335 to 1c5d35d Compare August 13, 2019 21:18

s-perron self-requested a review August 14, 2019 13:00

s-perron requested changes Aug 14, 2019

View reviewed changes

greg-lunarg force-pushed the inst53 branch 2 times, most recently from e35c8ea to fe8ac90 Compare August 14, 2019 20:50

Instrument: Add support for Buffer Device Address extension

001cf2c

greg-lunarg force-pushed the inst53 branch from fe8ac90 to 001cf2c Compare August 14, 2019 22:02

s-perron approved these changes Aug 16, 2019

View reviewed changes

s-perron merged commit 0640725 into KhronosGroup:master Aug 16, 2019

		@@ -0,0 +1,620 @@
		// Copyright (c) 2017 Valve Corporation
		// Copyright (c) 2017 LunarG Inc.

Conversation

greg-lunarg commented Aug 13, 2019

Uh oh!

greg-lunarg commented Aug 14, 2019

Uh oh!

s-perron commented Aug 14, 2019 via email

Uh oh!

s-perron left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-perron commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg-lunarg commented Aug 14, 2019

Uh oh!

greg-lunarg commented Aug 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

s-perron commented Aug 14, 2019 •

edited

Loading