Skip to content

Add a new class opt::CFG to represent the CFG for the module.#933

Closed
dnovillo wants to merge 3 commits intoKhronosGroup:masterfrom
dnovillo:const-prop
Closed

Add a new class opt::CFG to represent the CFG for the module.#933
dnovillo wants to merge 3 commits intoKhronosGroup:masterfrom
dnovillo:const-prop

Conversation

@dnovillo
Copy link
Copy Markdown
Contributor

This class moves some of the CFG-related functionality into a new
class opt::CFG. There is some other code related to the CFG in the
inliner and in opt::LocalSingleStoreElimPass that should also be moved,
but that require more changes than this pure restructuring.

I will move those bits in a follow-up PR.

Currently, the CFG is computed every time a pass is instantiated, but
this should be later moved to the new IRContext class that @s-perron is
working on.

@dnovillo dnovillo requested review from dneto0 and s-perron October 30, 2017 21:48
Copy link
Copy Markdown
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

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

A few minor comments. However, I think the TODOs should be removed before committing.

ii->ForEachInId([&f](const uint32_t* idp) { f(*idp); });
}

uint32_t BasicBlock::MergeBlockIdIfAny(uint32_t* cbid) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not that this needs to be done in this PR, but it is worth splitting this into two function? One to get the merge block, and another to get the continue block? It does not look like there is any looping going on. I won't fight for it if others disagree.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this function bugged me a bit because it does two distinct things. I've added BasicBlock::ContinueBlockIdIfAny().

if (cbid != nullptr) {
*cbid = 0;
}
uint32_t mbid = 0;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This code looks a lot like trying to fine the merge instruction. Could we call "GetMergeInst" instead?


// Return true if this block is a loop header block.
bool IsLoopHeader() const {
auto iItr = cend();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

could we just use "GetLoopMergeInst() != nullptr"? The other issue is that all of this only works for structured control flow. Will we want it to be more general than that? How should we let OpenCL people know they cannot use this function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

I've also asserted that we're processing a shader module in the SSA re-writer and the structured control compute.

source/opt/cfg.h Outdated
// block if it has one. When order matters, the merge block always appears
// first. This assures correct depth first search in the presence of early
// returns and kills. If the successor vector contain duplicates if the merge
// block, they are safely ignored by DFS. TODO(dnovillo): This belongs in a
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you remove the TODO now?

source/opt/cfg.h Outdated
// Compute structured block order for |func| into |structuredOrder|. This
// order has the property that dominators come before all blocks they
// dominate and merge blocks come after all blocks that are in the control
// constructs of their header. TODO(dnovillo): This belongs in a CFG class.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove the TODO if it is done.

@s-perron
Copy link
Copy Markdown
Collaborator

Also, the continuous builds are failing, so you need to check that out as well.

@dneto0
Copy link
Copy Markdown
Collaborator

dneto0 commented Oct 31, 2017

A bunch of changes were just pushed into master. Please rebase.

@dnovillo
Copy link
Copy Markdown
Contributor Author

A bunch of changes were just pushed into master. Please rebase.

Done.

This class moves some of the CFG-related functionality into a new
class opt::CFG.  There is some other code related to the CFG in the
inliner and in opt::LocalSingleStoreElimPass that should also be moved,
but that require more changes than this pure restructuring.

I will move those bits in a follow-up PR.

Currently, the CFG is computed every time a pass is instantiated, but
this should be later moved to the new IRContext class that @s-perron is
working on.
…IdIfAny

- Rewrite IsLoopHeader in terms of GetLoopMergeInst.
- Run clang-format on some files.
@dnovillo
Copy link
Copy Markdown
Contributor Author

dnovillo commented Nov 2, 2017

Re-based, re-tested, and pushed to master.

@dnovillo dnovillo closed this Nov 2, 2017
dneto0 pushed a commit to dneto0/SPIRV-Tools that referenced this pull request Sep 14, 2024
Roll third_party/glslang/ 0de87ee..6c47979 (4 commits)

KhronosGroup/glslang@0de87ee...6c47979

$ git log 0de87ee..6c47979 --date=short --no-merges --format='%ad %ae %s'
2019-12-09 cepheus Fix KhronosGroup#2020: PR KhronosGroup#1977 broke HLSL member consistency, this finishes it...
2019-12-09 cepheus Fix: KhronosGroup#2014: Don't do "extension-on && version >= ..." keyword checks.
2019-12-09 cepheus Fix KhronosGroup#2007: Fix a couple relative header paths in header files.
2019-12-09 cepheus Fix KhronosGroup#1993: Fully exclude ftransform() from SPIR-V semantics.

Roll third_party/googletest/ ae8d1fc81..78fdd6c00 (6 commits)

google/googletest@ae8d1fc...78fdd6c

$ git log ae8d1fc81..78fdd6c00 --date=short --no-merges --format='%ad %ae %s'
2019-12-05 absl-team Googletest export
2019-12-05 absl-team Googletest export
2019-12-05 absl-team Googletest export
2019-11-27 krystian.kuzniarek Revert "remove MSVC workaround: wmain link error in the static library"
2019-11-27 krystian.kuzniarek Revert "unify googletest and googlemock main functions"
2019-11-17 krystian.kuzniarek remove MSVC workaround: cease const dropping

Roll third_party/re2/ bb8e77755..6a86f6b3f (1 commit)

google/re2@bb8e777...6a86f6b

$ git log bb8e77755..6a86f6b3f --date=short --no-merges --format='%ad %ae %s'
2019-12-09 junyer Simplify the bytecode for the 80-10FFFF rune range.

Roll third_party/spirv-cross/ 15b860eb1..f912c3289 (2 commits)

KhronosGroup/SPIRV-Cross@15b860e...f912c32

$ git log 15b860eb1..f912c3289 --date=short --no-merges --format='%ad %ae %s'
2019-12-10 post GLSL: Fix array of input patch variables.
2019-12-09 post GLSL: Fix EmitStreamVertex/Primitive.

Roll third_party/spirv-tools/ e82a428..0a2b38d (2 commits)

KhronosGroup/SPIRV-Tools@e82a428...0a2b38d

$ git log e82a428..0a2b38d --date=short --no-merges --format='%ad %ae %s'
2019-12-10 afdx spirv-fuzz: function outlining fuzzer pass (KhronosGroup#3078)
2019-12-06 afdx spirv-fuzz: Use validator to check break/continue dominance conditions (KhronosGroup#3089)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants