This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Fix issues for backend divergence tracking
ClosedPublic

Authored by dstuttard on Apr 6 2018, 8:27 AM.

Details

Summary

A change to use divergence analysis in the AMDGPU backend was getting formal
arguments incorrect (not tagged as divergent) unless they were VGPR0, VGPR1 or
VGPR2

For graphics shaders it is possible to have more than these passed in as VGPR

Modified the checking code to check for any VGPR registers passed in as formal
arguments.

Also, some intrinsics that are sources of divergence may have been lowered
during instruction selection and are missed on subsequent calls to
isSDNodeSourceOfDivergence - added the relevant AMDGPUISD checks as well.

Finally, the FunctionLoweringInfo tracks virtual registers that are live across
basic block boundaries. This is used to check for divergence of CopyFromRegister
registers using the DivergenceAnalysis analysis. For multiple blocks the lazily
evaluated inverted map VirtReg2Value was not cleared when the ValueMap map was.

Diff Detail

Event Timeline

dstuttard created this revision.Apr 6 2018, 8:27 AM
arsenm added inline comments.Apr 6 2018, 8:40 AM
test/CodeGen/AMDGPU/diverge-extra-formal-args.ll
11

Move the triple to run line and delete it and the datalayout

test/CodeGen/AMDGPU/diverge-interp-mov-lower.ll
8

Ditto

dstuttard updated this revision to Diff 141372.Apr 6 2018, 9:44 AM

Folded in triple to run line

dstuttard marked 2 inline comments as done.Apr 6 2018, 9:45 AM
arsenm accepted this revision.Apr 6 2018, 1:26 PM

LGTM

test/CodeGen/AMDGPU/diverge-multi-func.ll
9

Missing check?

This revision is now accepted and ready to land.Apr 6 2018, 1:26 PM
dstuttard marked an inline comment as done.Apr 7 2018, 1:40 AM
dstuttard added inline comments.
test/CodeGen/AMDGPU/diverge-multi-func.ll
9

No - this function is here purely to provoke the bug in the geometry shader, so it doesn't need a check as we don't actually care what is emitted.

arsenm added inline comments.Apr 9 2018, 6:22 PM
test/CodeGen/AMDGPU/diverge-multi-func.ll
9

How does this change an unrelated function?

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?

alex-t accepted this revision.Apr 10 2018, 6:37 AM
dstuttard marked an inline comment as done.Apr 16 2018, 7:11 AM

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.
  1. Remove the test altogether since the VirtReg2Value.clear() is definitely required and was originally missed as an oversight - and the test might be overkill.

Sounds reasonable. It really was an oversight :) It's funny that the change was reviewed by lots of people for 2 months but nobody noticed not clearing state.

  1. Remove the test altogether since the VirtReg2Value.clear() is definitely required and was originally missed as an oversight - and the test might be overkill.

Sounds reasonable. It really was an oversight :) It's funny that the change was reviewed by lots of people for 2 months but nobody noticed not clearing state.

Ok - I agree. I'll remove that test and submit the change. Thanks.

dstuttard updated this revision to Diff 142640.Apr 16 2018, 8:46 AM

Removed test as being overkill for the missed state clear being added (2 other
tests still remain)

This revision was automatically updated to reflect the committed changes.