[PATCH] D45372: [AMDGPU] Fix issues for backend divergence tracking

David Stuttard via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Apr 16 07:12:59 PDT 2018


dstuttard marked an inline comment as done.
dstuttard added a comment.

In https://reviews.llvm.org/D45372#1062947, @alex-t wrote:

> In general LGTM.
>  I also concerned about the magic test that has no checks and has no visible side effects on shared data but is necessary to reproduce buggy behavior.
>  Could you please clarify what do you need it for?


Hopefully answering both Matt and Alex's concern about the magic test. The test is a bugpoint cut-down of a failing shader test (then modified by hand).
The fix that adds the VirtReg2Value.clear() is tested by this test.
This missing clear means that the lazily evaluated VirtReg2Value was being created in the first function (the untested PS shader function in this case) and then wasn't cleared for the second GS function (and hence contains the wrong mappings - for the PS function and not the GS). It doesn't really matter what the first function does, as long as it creates the VirtReg2Value mapping - the content of this function does this.
Removing the first function stops the test exposing this bug.
Hopefully this is more clear now.

I can do a couple of things if you think it worthwhile:

1. Add some more comments to the test to make it clearer why there is a "magic" untested function in the test.
2. Remove the test altogether since the VirtReg2Value.clear() is definitely required and was originally missed as an oversight - and the test might be overkill.


Repository:
  rL LLVM

https://reviews.llvm.org/D45372





More information about the llvm-commits mailing list