This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix up memory ordering after combining BV to a load
ClosedPublic

Authored by nemanjai on Dec 14 2022, 12:24 PM.

Details

Summary

The combiner for BUILD_VECTOR that merges consecutive loads into a wide load had two issues:

  • It didn't check that the input loads all have the same input chain
  • It didn't update nodes that are chained to the original loads to be chained to the new load

This caused issues with bootstrap when 3c4d2a03968ccf5889bacffe02d6fa2443b0260f was committed. This patch fixes the issue so it can unblock this commit.

Diff Detail

Event Timeline

nemanjai created this revision.Dec 14 2022, 12:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2022, 12:24 PM
nemanjai requested review of this revision.Dec 14 2022, 12:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2022, 12:24 PM

@nemanjai thank you *so* much for dealing with this miscompile!

RKSimon accepted this revision.Dec 14 2022, 2:01 PM

LGTM, although its annoying that you can't use SelectionDAG::areNonVolatileConsecutiveLoads somehow.

One minor - nothing to do with this patch, but something that probably should be addressed if you're in the vicinity.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14280–14281

You're immediately dereferencing dyn_cast<> - these can probably be cast<>?

This revision is now accepted and ready to land.Dec 14 2022, 2:01 PM
efriedma added inline comments.Dec 14 2022, 2:04 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14263

Do we really need a SmallPtrSet here? We check each load exactly once, so we should be able to just use a SmallVector.

Iterating over a SmallPtrSet like this might lead to non-determinism.

LGTM, although its annoying that you can't use SelectionDAG::areNonVolatileConsecutiveLoads somehow.

One minor - nothing to do with this patch, but something that probably should be addressed if you're in the vicinity.

I think we just missed SelectionDAG::areNonVolatileConsecutiveLoads in the initial implementation. We should be able to use that, but I can refactor that in a follow-up.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14263

Sounds good. I'll change it to SmallVector.

14280–14281

Sure, I can change that while I'm here.

nemanjai updated this revision to Diff 483329.Dec 15 2022, 1:59 PM

Addressed review comments:

  • Switch from SmallPtrSet to SmallVector
  • Used areNonVolatileConsecutiveLoads() and updated it to consider the memory VT vs. the loaded VT
  • Switched dyn_cast to cast for converting nodes whose opcodes were already checked
nemanjai marked 2 inline comments as done.Dec 15 2022, 2:01 PM
nemanjai added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
11500

@efriedma @RKSimon Do you see any issues with switching this? It seems that we should be checking the memory type to determine if loads are consecutive. This is also required in order to use this query for extending loads for PPC.

nemanjai updated this revision to Diff 483341.Dec 15 2022, 2:18 PM

Forgot to remove the redundant checks for input chain equality now that I switched to areNonVolatileConsecutiveLoads().

efriedma accepted this revision.Dec 15 2022, 2:56 PM

LGTM

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
11500

I think all the existing uses exclude extending loads? Seems fine.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14309

Is LD1 here the same node as LD above? Could simplify that a bit, maybe.

RKSimon added inline comments.Dec 16 2022, 7:02 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
11500

an assertion/earlyout for extend loads might make sense?

YAY! Thank you!
I'm going to reland SROA patch, hopefully all bots stay happy this time :)

@nemanjai looks like i may have some rather unfortunate news:
https://lab.llvm.org/buildbot/#/builders/93/builds/12560
I'm waiting for https://lab.llvm.org/buildbot/#/builders/93/builds/12561 to finish, and if it suddenly doesn't succeed, revert the recommit...

@nemanjai looks like i may have some rather unfortunate news:
https://lab.llvm.org/buildbot/#/builders/93/builds/12560
I'm waiting for https://lab.llvm.org/buildbot/#/builders/93/builds/12561 to finish, and if it suddenly doesn't succeed, revert the recommit...

Silver-lining: little-endian PPC bots appear to be green: https://lab.llvm.org/buildbot/#/builders/121 https://lab.llvm.org/buildbot/#/builders/36
So the remaining problem is limited to big-endian.

@nemanjai looks like i may have some rather unfortunate news:
https://lab.llvm.org/buildbot/#/builders/93/builds/12560
I'm waiting for https://lab.llvm.org/buildbot/#/builders/93/builds/12561 to finish, and if it suddenly doesn't succeed, revert the recommit...

Silver-lining: little-endian PPC bots appear to be green: https://lab.llvm.org/buildbot/#/builders/121 https://lab.llvm.org/buildbot/#/builders/36
So the remaining problem is limited to big-endian.

@lebedev.ri Did your original commit have this problem on the big endian bot?

@nemanjai looks like i may have some rather unfortunate news:
https://lab.llvm.org/buildbot/#/builders/93/builds/12560
I'm waiting for https://lab.llvm.org/buildbot/#/builders/93/builds/12561 to finish, and if it suddenly doesn't succeed, revert the recommit...

Silver-lining: little-endian PPC bots appear to be green: https://lab.llvm.org/buildbot/#/builders/121 https://lab.llvm.org/buildbot/#/builders/36
So the remaining problem is limited to big-endian.

@lebedev.ri Did your original commit have this problem on the big endian bot?

Yup: https://lab.llvm.org/buildbot/#/builders/93/builds/12251
To be honest, i didn't see that particular report :/

@lebedev.ri Did your original commit have this problem on the big endian bot?

Yup: https://lab.llvm.org/buildbot/#/builders/93/builds/12251
To be honest, i didn't see that particular report :/

OK, give me a couple of hours to see if I can track this down. At least I know what the likely culprit is so it's worth looking there. If I can't fix it right away, I'll revert your patch, get the PPC back end fixed and let you know.

@lebedev.ri Did your original commit have this problem on the big endian bot?

Yup: https://lab.llvm.org/buildbot/#/builders/93/builds/12251
To be honest, i didn't see that particular report :/

OK, give me a couple of hours to see if I can track this down. At least I know what the likely culprit is so it's worth looking there. If I can't fix it right away, I'll revert your patch, get the PPC back end fixed and let you know.

Thank you for taking a look!
I'll revert the patch regardless for now, looks like i need to fix commit message there.