This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] improve fragile test for divergent branches
ClosedPublic

Authored by sameerds on Feb 27 2020, 8:47 AM.

Details

Summary

The affected LIT test intends to test the correct use of divergence
analysis to detect a divergent branch with a uniform predicate. The
passes involved are LLVM IR passes, but the test runs llc and tries to
match against generated ISA, which makes it hard to demonstrate that
the intended behavior was really tested. Replaced this with a test
that invokes opt on the required passes and then checks for the
appropriate changes in the LLVM IR.

Diff Detail

Event Timeline

sameerds created this revision.Feb 27 2020, 8:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2020, 8:47 AM
arsenm added inline comments.Feb 27 2020, 8:52 AM
llvm/test/CodeGen/AMDGPU/divergent-branch-uniform-condition.ll
8

I think this should include ISA generated checks, and we should probably just generate checks for all control flow tests

sameerds added inline comments.Feb 27 2020, 8:58 AM
llvm/test/CodeGen/AMDGPU/divergent-branch-uniform-condition.ll
8

I am not sure I understand what you mean. Do you want ISA checks in addition to the checks that I am proposing? Also, what is the set of "all control flow tests"?

38

Note that this block does not even exist in the original test. It is generated internally by the structurizer. Any changes to the structurizer can potentially stop generating the branch that is being implied in the original test.

arsenm added inline comments.Feb 27 2020, 9:02 AM
llvm/test/CodeGen/AMDGPU/divergent-branch-uniform-condition.ll
8

Yes. I mean in general we should just generate checks for any control flow test, which this is one of

sameerds marked an inline comment as done.Feb 27 2020, 9:16 AM
sameerds added inline comments.
llvm/test/CodeGen/AMDGPU/divergent-branch-uniform-condition.ll
8

Yeah, I see the point in general. But the problem with this test is that there is no line in the original input that explicitly captures what is being tested. (See my inline comment elsewhere about the block Flow2.) The new version is a dump of the LLVM IR after structurizer to actually show the branch being tested. Using this version to generate ISA would pass it through the structurizer yet again, which would need a very strong promise on the structurizer being idempotent. It's also possible that something else in the whole codegen flow might make the output insensitive to whether this specific divergent branch is correctly recognized.

Essentially, there are just too many things happening from input to output. As it stands, the new version of the test makes it a proper "unit test" that explicitly shows what is being tested. It's not the code generation that is being examined here, but the handling of divergent branches by specific LLVM IR passes before code generation.

arsenm accepted this revision.Feb 27 2020, 9:23 AM
This revision is now accepted and ready to land.Feb 27 2020, 9:23 AM
sameerds updated this revision to Diff 246986.Feb 27 2020, 9:26 AM

fixed the incorrect use of NOT and NEXT directives in FileCheck

This revision was automatically updated to reflect the committed changes.