Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

spirv-fuzz: All instructions in a dead block are irrelevant #3733

Closed
Vasniktel opened this issue Aug 21, 2020 · 4 comments · Fixed by #3795
Closed

spirv-fuzz: All instructions in a dead block are irrelevant #3733

Vasniktel opened this issue Aug 21, 2020 · 4 comments · Fixed by #3795
Assignees
@stefanomil
Labels
component:fuzzer Relates to the spirv-fuzz tool

Comments

@Vasniktel
Copy link
Collaborator

Consider marking all instructions in dead blocks as irrelevant.
@afd afd added the component:fuzzer Relates to the spirv-fuzz tool label Sep 8, 2020
@afd
Copy link
Contributor

afd commented Sep 8, 2020

@stefanomil Let's discuss this when you're ready to work on it.

@stefanomil
Copy link
Collaborator

I can think of two approaches for this:

(1)
Change the IdIsIrrelevant and GetIrrelevantIds functions, so that, in addition to checking the irrelevant_ids_ set, they also consider irrelevant any instruction that is in a dead block. (This will require adding the context to the parameters of these functions)

Pro: any new instructions added to a dead block will always be considered irrelevant
Con: GetIrrelevantIds cannot just return a const reference to the set of irrelevant ids anymore, but it will need to copy it and add any additional ids from dead blocks

(2)
Change the AddFactBlockIsDead, so that it adds an IrrelevantId fact for each id in the block. (This will require adding the context to the parameters of this function)

Pro?: if instructions are moved out of the block, they will still be considered irrelevant (I'm not sure if this is actually desirable, though)

@afd
Copy link
Contributor

afd commented Sep 10, 2020

I think option (1) is good.

I think we would like instructions that get migrated out of dead blocks to be considered irrelevant, but we could handle that by checking for it and adding explicit IrrelevantId facts when such movement happens.

@Vasniktel what do you think? Some of your transformations move instructions around.

@stefanomil stefanomil mentioned this issue Sep 11, 2020
Merged
@Vasniktel
Copy link
Collaborator Author

Vasniktel commented Sep 15, 2020 •
edited
Loading

I'm OK with (1). Regarding the need for the IR context as an argument, I'm going to refactor the fact manager to take this into account.

@Vasniktel Vasniktel mentioned this issue Sep 15, 2020
Open
@afd afd closed this as completed in #3795 Sep 18, 2020
afd pushed a commit that referenced this issue Sep 18, 2020
@stefanomil
spirv-fuzz: Consider all ids from dead blocks irrelevant (#3795)
2945963 
This PR modifies the FactManager methods IdIsIrrelevant and GetIrrelevantIds so
that an id is always considered irrelevant if it comes from a dead block.

Fixes #3733.
sudonatalie added a commit that referenced this issue Apr 4, 2022
@sudonatalie
Roll external/googletest/ f45d5865e..25dcdc7e8 (26 commits)
047db97 
google/googletest@f45d586...25dcdc7

$ git log f45d5865e..25dcdc7e8 --date=short --no-merges --format='%ad %ae %s'
2022-04-04 dmauro Use the Abseil flags library when Abseil is present
2022-03-23 absl-team Address deprecation warning surfaced by Github presubmit tests
2022-03-23 mattias.ellert Split gmock-matchers_test into 4 smaller test #3653
2022-03-22 absl-team Only print disabled test banner if the test matches gtest_filter
2022-03-21 absl-team Clarify public access on gmock examples.
2022-03-18 bmesser Remove sanity as it is offensive to neurodiverse individuals.
2022-03-15 absl-team Running clang-format over all of GoogleTest
2022-03-14 dinor Remove references to deleted script gen_gtest_pred_impl.py
2022-03-08 absl-team Mark ACTION_Pn()-generated functions as must-use-result, adding non-compilation tests.
2022-03-08 sobik.szymon Add myself to contributors
2022-03-08 sobik.szymon Adjust documentation regarding xml and json source file location otput.
2022-03-08 sobik.szymon Adjust xml and json unit tests to test for source file and line location.
2022-03-08 sobik.szymon Add support for testing of xml and json output of source file and line location
2022-03-08 sobik.szymon Output source file path and line number in xml and json files.
2022-02-17 dmauro Update GCC/Clang Linux tests to use Bazel 5.0.0
2022-02-14 absl-team Address conversion warning by explicitly casting to size_t
2022-02-09 absl-team Add a 3-arg overload for ResultOf() matcher that takes a description string for better error messages.
2022-02-05 hgsilverman Apply requested changes by using std::inserter with move.
2022-02-05 noiseless-ak Fix gtest-help-test failure on OpenBSD
2022-02-01 absl-team GetCurrentOsStackTraceExceptTop (both the method of UnitTestImpl and the wrapper function in gtest.cc) rely on the fact that the inner call is not getting optimized. This CL annotates them with the appropriate attributes.
2022-01-29 hgsilverman Do constant time matching for exact match filters.
2022-01-28 dmauro Finish some missed pieces of the TestCase to TestSuite Migration
2022-01-26 dinor Change `ReturnArg` to use perfect forwarding of arguments (#3733)
2022-01-25 melroy Let me give a change to try it again - updating to latest version
2022-01-05 73706994+jjfvanderpol Set CMake Policy CMP0077 to NEW
2021-10-19 deniz CMake: Fix values of INTERFACE_INCLUDE_DIRECTORIES property

Created with:
  roll-dep external/googletest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stefanomil stefanomil

Labels
component:fuzzer Relates to the spirv-fuzz tool
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

spirv-fuzz: Consider all ids from dead blocks irrelevant stefanomil/SPIRV-Tools
3 participants
@afd @Vasniktel @stefanomil