This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Turn on CR-Logical reducer pass
ClosedPublic

Authored by nemanjai on Sep 24 2018, 12:06 PM.

Details

Summary

Quite a while ago, we implemented a pass that will reduce the number of CR-logical operations we emit. It does so by converting a CR-logical operation into a branch. We have kept this off by default because it seemed to cause a significant regression with one benchmark.
However, that regression turned out to be due to a completely unrelated reason - AADB introducing a self-copy that is a priority-setting nop and it was just exacerbated by this pass.

Now that we understand the reason for the only degradation, we can turn this pass on by default. A subsequent patch will detect and remove self-copies in the pre-emit peephole.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Sep 24 2018, 12:06 PM
hfinkel accepted this revision.Sep 25 2018, 7:01 AM

Now that we understand the reason for the only degradation, we can turn this pass on by default. A subsequent patch will detect and remove self-copies in the pre-emit peephole.

SGTM.

This revision is now accepted and ready to land.Sep 25 2018, 7:01 AM
This revision was automatically updated to reflect the committed changes.
nemanjai added a comment.EditedOct 18 2019, 5:46 AM

Sorry about that. I'll have a look. I don't think it is the cause of the AMDGPU failures though.

Sorry about that. I'll have a look. I don't think it is the cause of the AMDGPU failures though.

Turns out I cannot build with EXPENSIVE_CHECKS turned on with my setup on PPC Linux. I'll revert the change until I can get this sorted out.

Sorry about that. I'll have a look. I don't think it is the cause of the AMDGPU failures though.

No, they were already a known problem: https://bugs.llvm.org/show_bug.cgi?id=43699

@RKSimon What are the chances I could get access to a machine where these issues can be reproduced? I have tried building with EXPENSIVE_CHECKS on PPC Linux, but that fails and I tried it on my MacBook but I am not seeing these failures with the patch applied.

OTOH, I see that on that bot, you build a debug build. I wonder if this failure only happens on debug builds with EXPENSIVE_CHECKS... I'll try that on my machine now.

RKSimon reopened this revision.Oct 19 2019, 7:37 AM

@nemanjai Any luck on this? It seems to be something to do with PPCReduceCRLogicals::splitBlockOnBinaryCROp AllCRLogicalOps.push_back() calls, resulting in a reallocation and the original CRLogicalOpInfo &CRI reference being lost. Should/can the CRLogicalOpInfo& be passed by value instead?

This revision is now accepted and ready to land.Oct 19 2019, 7:37 AM
RKSimon requested changes to this revision.Oct 19 2019, 7:38 AM

This was reverted in rL375233

This revision now requires changes to proceed.Oct 19 2019, 7:38 AM

@nemanjai Any luck on this? It seems to be something to do with PPCReduceCRLogicals::splitBlockOnBinaryCROp AllCRLogicalOps.push_back() calls, resulting in a reallocation and the original CRLogicalOpInfo &CRI reference being lost. Should/can the CRLogicalOpInfo& be passed by value instead?

Ah, this makes perfect sense. Thanks Simon. I'll try to put together a reproducer and a fix.

nemanjai planned changes to this revision.Oct 19 2019, 8:59 AM
nemanjai updated this revision to Diff 225762.Oct 19 2019, 11:53 AM

A few updates to the CR Logical Reducer in addition to the pulled patch that turns it on:

  • Do not use std::vector as there is no real reason for it
  • Do not take references into a growable data structure :(

@RKSimon Simon, could you do me a big favour and try this out on the machine on which you saw the failures. I am not sure if this is the (only) issue but references into a growable data structure certainly should be removed. Thanks for spotting this issue - I don't know what I was thinking when I originally wrote this a long time ago.

A few updates to the CR Logical Reducer in addition to the pulled patch that turns it on:

  • Do not use std::vector as there is no real reason for it
  • Do not take references into a growable data structure :(

@RKSimon Simon, could you do me a big favour and try this out on the machine on which you saw the failures. I am not sure if this is the (only) issue but references into a growable data structure certainly should be removed. Thanks for spotting this issue - I don't know what I was thinking when I originally wrote this a long time ago.

I'll test it out but I'll be travelling to the devmtg so its not going to be until Monday at the earliest.

RKSimon accepted this revision.Oct 21 2019, 10:43 AM

Thanks @nemanjai this no longer asserts/crashes on my windows build

This revision is now accepted and ready to land.Oct 21 2019, 10:43 AM

Thanks @nemanjai this no longer asserts/crashes on my windows build

Thank you so much for all your help sorting this out Simon.

This revision was automatically updated to reflect the committed changes.