Page MenuHomePhabricator

SIFixSGPRCopies should not change non-divergent PHI
ClosedPublic

Authored by alex-t on Nov 28 2017, 6:38 AM.

Details

Summary

According to the comment in the code SIFixSGPRCopies claims that it check if PHI is uniform by examination of the terminator of the nearest common dominator of the joined values.

  1. nearest common dominator check is not sufficient for control dependency analysis (see https://www.cs.rice.edu/~keith/EMBED/dom.pdf for details)
  2. Current code does not even does what the comments claims. It really turn PHI (and all it's uses) to vector if any of the PHI's parent block predecessors is divergent.

This change introduces post-dominance frontiers computing procedure and uses the results to decide if the PHI is divergent.

Diff Detail

Event Timeline

alex-t created this revision.Nov 28 2017, 6:38 AM
rampitec added inline comments.Nov 28 2017, 12:00 PM
lib/Target/AMDGPU/SIFixSGPRCopies.cpp
116

Check for NDEBUG here.

arsenm added inline comments.Nov 28 2017, 12:12 PM
lib/Target/AMDGPU/SIFixSGPRCopies.cpp
391–422

These aren't used anymore and should be deleted

571

Reformat entire function

600

Single quotes

602

Single quotes

680–681

References?

alex-t updated this revision to Diff 124733.Nov 29 2017, 6:04 AM

updated according to the comments

This revision is now accepted and ready to land.Nov 29 2017, 11:07 AM
This revision was automatically updated to reflect the committed changes.

Hi guys,

This change appears to introduce a regression. It hangs nier:automata with the RADV Vulkan driver, reverting the commit fixes the issue.

Here's the full LLVM IR of that shader https://hastebin.com/minayakiri

I have also tried to reduce the testcase, but I'm not 100% sure it's correct, in case you want to have a look too https://hastebin.com/olatenegej

After comparing with/without that change, it appears that some VGPR PHIs are SGPR PHIs now.

I know this is why this change has been submitted initially (ie. to not use VGPR PHIs when the control flow is uniform), but I wonder if the new postdominance frontier is correct is *all* situations.

The concept of VGPR/SGPR PHIs seems tricky and I'm not sure to fully understand it.

Can you check (and/or explain) if the SGPR PHIs in that shader are expected?

Thanks in advance,
Samuel.

alex-t added a subscriber: arsenm.Mar 15 2018, 9:29 AM

Hi Samuel,

sure I will look at this. I'll let you know when I get an idea of what
going wrong there.

Alexander

Is this related to D40547? I sent a ping on that, but no response...

I m looking on this.

17 марта 2018 г. 3:27 PM пользователь "Nicolai Hähnle via Phabricator" <
reviews@reviews.llvm.org> написал:

nhaehnle added a comment.

Is this related to https://reviews.llvm.org/D40547? I sent a ping on
that, but no response...

Repository:

rL LLVM

https://reviews.llvm.org/D40556

alex-t added a comment.Apr 9 2018, 4:37 AM

It seems like this patch should be reverted. Recently we have no reliable
way to determine the PHI (or whatever else) divergence on the MI level.
The only correct way is to add the DA algorithm that re-computes all the MI
divergence.
Since we're going to remove the SGPRFix stuff as soon as diverence driven
ISel is ready I consider this patch is not necessary.

It seems like this patch should be reverted. Recently we have no reliable
way to determine the PHI (or whatever else) divergence on the MI level.
The only correct way is to add the DA algorithm that re-computes all the MI
divergence.
Since we're going to remove the SGPRFix stuff as soon as diverence driven
ISel is ready I consider this patch is not necessary.

Thanks for looking into this. The main issue I see with a revert is that we will need a backport to LLVM 6.

Though, if it's the only solution for now, this sounds good to me.

In fact yes. This is the most correct solution. The stuff in
SIFixSGPRCopies.cpp has got to gone.
So it does not make sense to spend efforts on it. Moreover there is no
correct solution at all
unless we implement one more DivergenceAnalysis upon the machine IR.
So, if it's okay I'd prefer to revert.

In fact yes. This is the most correct solution. The stuff in
SIFixSGPRCopies.cpp has got to gone.
So it does not make sense to spend efforts on it. Moreover there is no
correct solution at all
unless we implement one more DivergenceAnalysis upon the machine IR.
So, if it's okay I'd prefer to revert.

I'm fine with the revert because this change introduces hangs in a bunch of games apparently.

Do you think it's doable to backport the revert to LLVM 6 as well?

Thanks!

I have no idea how to backport yet.
I 'my going to submit revert patch.
This change existed for a few months and I' my not sure what may be broken
if reverted. I have to figure out this. Some precheckin tests needed.

пт, 13 апр. 2018 г., 10:36 Samuel Pitoiset via Phabricator <
reviews@reviews.llvm.org>:

hakzsam added a comment.

In https://reviews.llvm.org/D40556#1062962, @alex-t wrote:

> In fact yes. This is the most correct solution. The stuff in
> SIFixSGPRCopies.cpp has got to gone.
> So it does not make sense to spend efforts on it. Moreover there is no
> correct solution at all
> unless we implement one more DivergenceAnalysis upon the machine IR.
> So, if it's okay I'd prefer to revert.

I'm fine with the revert because this change introduces hangs in a bunch
of games apparently.

Do you think it's doable to backport the revert to LLVM 6 as well?

Thanks!

Repository:

rL LLVM

https://reviews.llvm.org/D40556

Hi,

Any news on the revert? Thanks!

Sorry for the delay. Just submitted to trunk.
It was not about the reverse patch only. I had to change tests accordingly.

Sorry for the delay. Just submitted to trunk.
It was not about the reverse patch only. I had to change tests accordingly.

Thanks a lot!