This is an archive of the discontinued LLVM Phabricator instance.

StructurizeCFG: Test for branch divergence correctly
ClosedPublic

Authored by nhaehnle on Feb 25 2018, 7:25 AM.

Details

Summary

Fixes cases like the new test @nonuniform. In that test, %cc itself
is a uniform value; however, when reading it after the end of the loop in
basic block %if, its value is effectively non-uniform, so the branch is
non-uniform.

This problem was encountered in
https://bugs.freedesktop.org/show_bug.cgi?id=103743; however, this change
in itself is not sufficient to fix that bug, as there is another issue
in the AMDGPU backend.

As discovered after committing an earlier version of this change, this
exposes a subtle interaction between this pass and DivergenceAnalysis:
since we remove and re-create branch instructions, we can no longer rely
on DivergenceAnalysis for branches in subregions that were already
processed by the pass.

Explicitly remove branch instructions from DivergenceAnalysis to
avoid dangling pointers as a matter of defensive programming, and
change how we detect non-uniform subregions.

Change-Id: I32bbffece4a32f686fab54964dae1a5dd72949d4

Diff Detail

Event Timeline

nhaehnle created this revision.Feb 25 2018, 7:25 AM
dberlin added inline comments.
lib/Transforms/Scalar/StructurizeCFG.cpp
895

auto please

897

auto

916

Auto here too

934

Why is this being set to null here?

nhaehnle updated this revision to Diff 135832.Feb 25 2018, 9:07 AM
nhaehnle marked 2 inline comments as done.

Use auto.

lib/Transforms/Scalar/StructurizeCFG.cpp
934

DA is a member variable that needs to be re-initialized on every run of the pass. This is just like the other analyses, DT and LI.

It is set to null because it is optional and not always available. It is only used on specific targets.

jlebar edited reviewers, added: timshen; removed: jlebar.Mar 23 2018, 4:58 AM
jlebar added a subscriber: jlebar.

@timshen is looking at turning on the structurizer for NVPTX, since we keep hitting bugs in ptxas that we think/hope will be solved by this.

arsenm added inline comments.Mar 26 2018, 8:10 AM
lib/Transforms/Scalar/StructurizeCFG.cpp
938–943

This seems to be increasing the reliance on metadata? Is this passing information from one RegionPass invocation to another? Metadata isn't really intended to be way to pass information like that

arsenm added inline comments.Mar 26 2018, 9:34 AM
lib/Transforms/Scalar/StructurizeCFG.cpp
938–943

I have a patch to stop using a RegionPass and instead just use the RegionInfo directly in a FunctionPass. Would that help avoid this?

alex-t accepted this revision.Mar 27 2018, 6:03 AM
This revision is now accepted and ready to land.Mar 27 2018, 6:03 AM
nhaehnle added inline comments.Mar 27 2018, 8:43 AM
lib/Transforms/Scalar/StructurizeCFG.cpp
938–943

I think the metadata issue here is minimal, because we're only looking for metadata that this pass adds itself. But yes, moving towards a FunctionPass would certainly help clean this up.

arsenm added inline comments.Mar 27 2018, 12:04 PM
lib/Transforms/Scalar/StructurizeCFG.cpp
938–943

I think this at least needs a comment explicitly mentioning how this is passing info between pass invocations and needs to be fixed

This revision was automatically updated to reflect the committed changes.
nhaehnle marked an inline comment as done.Apr 4 2018, 4:03 AM
nhaehnle added inline comments.
lib/Transforms/Scalar/StructurizeCFG.cpp
938–943

I've expanded on the "TODO" comment before committing.