This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG][Mips][PowerPC][RISCV][WebAssembly] Teach computeKnownBits/ComputeNumSignBits about atomics
ClosedPublic

Authored by jrtc27 on Apr 26 2021, 9:33 PM.

Details

Summary

Unlike normal loads these don't have an extension field, but we know
from TargetLowering whether these are sign-extending or zero-extending,
and so can optimise away unnecessary extensions.

This was noticed on RISC-V, where sign extensions in the calling
convention would result in unnecessary explicit extension instructions,
but this also fixes some Mips inefficiencies. PowerPC sees churn in the
tests as all the zero extensions are only for promoting 32-bit to
64-bit, but these zero extensions are still not optimised away as they
should be, likely due to i32 being a legal type.

This also simplifies the WebAssembly code somewhat, which currently
works around the lack of target-independent combines with some ugly
patterns that break once they're optimised away.

Diff Detail

Event Timeline

jrtc27 created this revision.Apr 26 2021, 9:33 PM
jrtc27 requested review of this revision.Apr 26 2021, 9:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2021, 9:33 PM
jrtc27 added inline comments.Apr 26 2021, 9:38 PM
llvm/test/CodeGen/RISCV/atomic-signext.ll
19

NB: These tests aren't in the tree, this is just showing the diff to my local pre-commit.

The SelectionDAG changes LGTM.

@jrtc27 Please can you pre-commit your local tests and then rebase this patch?

jrtc27 updated this revision to Diff 342686.May 4 2021, 3:14 AM

Rebased with tests pre-committed

RKSimon accepted this revision.May 4 2021, 3:59 AM

LGTM - please can you raise a bug detailing the PPC poor codegen for i32/i64 promotions

This revision is now accepted and ready to land.May 4 2021, 3:59 AM
jrtc27 added a comment.May 4 2021, 4:08 AM

LGTM - please can you raise a bug detailing the PPC poor codegen for i32/i64 promotions

Filed as https://bugs.llvm.org/show_bug.cgi?id=50217. Does this need a review from a WASM person given the changes there?

RKSimon added a subscriber: tlively.May 4 2021, 4:24 AM

@tlively @sunfish Any final comments on the wasm change?

The Wasm changes LGTM! This is a nice simplification.

atanasyan accepted this revision.May 5 2021, 1:31 AM

The MIPS changes LGTM

This revision was landed with ongoing or failed builds.May 5 2021, 8:35 AM
This revision was automatically updated to reflect the committed changes.
jrtc27 reopened this revision.May 5 2021, 9:02 AM

Reverted due to assertion failures during sanitiser builds on the buildbots. Will try and reproduce+debug over the coming days.

This revision is now accepted and ready to land.May 5 2021, 9:02 AM

LGTM - please can you raise a bug detailing the PPC poor codegen for i32/i64 promotions

Filed as https://bugs.llvm.org/show_bug.cgi?id=50217. Does this need a review from a WASM person given the changes there?

Thanks for opening the bug. This is a fairly long standing issue in the PPC back end and isn't specific to atomics. We do plan to address it when we find time.

This revision was automatically updated to reflect the committed changes.

@jrtc27
It looks like this patch has broken one of out Big Endian PowerPC buildbots:
https://lab.llvm.org/buildbot/#/builders/93/builds/3021

We are currently investigating but if we cannot find the solution quickly we will pull the patch and continue investigating after that.

@jrtc27
It looks like this patch has broken one of out Big Endian PowerPC buildbots:
https://lab.llvm.org/buildbot/#/builders/93/builds/3021

We are currently investigating but if we cannot find the solution quickly we will pull the patch and continue investigating after that.

The only thing I can think of is if any atomics are lowered with sign-extending rather than zero-extending instructions, thereby violating what getExtendForAtomicOps claims?

stefanp reopened this revision.May 12 2021, 7:49 AM

Hi @jrtc27

I apologize but we have not been able to get to the bottom of this quickly. As a result we have pulled the change with:

commit 8d37411e48202b490c62ee3548df4b90f5974e12 (HEAD -> main, origin/main, origin/HEAD)
Author: Stefan Pintilie <stefanp@ca.ibm.com>
Date:   Wed May 12 09:42:09 2021 -0500

    Revert "[SelectionDAG][Mips][PowerPC][RISCV][WebAssembly] Teach computeKnownBits/ComputeNumSignBits about atomics"
    
    This reverts commit 6c80361b8474535852afb2f7201370fb5f410091.
    Breaks PowerPC Big Endian buildbots.

I will keep you up to date as we have more information.
I have reopened this review.

This revision is now accepted and ready to land.May 12 2021, 7:49 AM

Hi @jrtc27

I apologize but we have not been able to get to the bottom of this quickly. As a result we have pulled the change with:

commit 8d37411e48202b490c62ee3548df4b90f5974e12 (HEAD -> main, origin/main, origin/HEAD)
Author: Stefan Pintilie <stefanp@ca.ibm.com>
Date:   Wed May 12 09:42:09 2021 -0500

    Revert "[SelectionDAG][Mips][PowerPC][RISCV][WebAssembly] Teach computeKnownBits/ComputeNumSignBits about atomics"
    
    This reverts commit 6c80361b8474535852afb2f7201370fb5f410091.
    Breaks PowerPC Big Endian buildbots.

I will keep you up to date as we have more information.
I have reopened this review.

I have access to big-endian PowerPC hardware. Is the reproducer as simple as running the compiler-rt test suite?

stefanp added a comment.EditedMay 13 2021, 3:27 AM

Hi @jrtc27

I apologize but we have not been able to get to the bottom of this quickly. As a result we have pulled the change with:

commit 8d37411e48202b490c62ee3548df4b90f5974e12 (HEAD -> main, origin/main, origin/HEAD)
Author: Stefan Pintilie <stefanp@ca.ibm.com>
Date:   Wed May 12 09:42:09 2021 -0500

    Revert "[SelectionDAG][Mips][PowerPC][RISCV][WebAssembly] Teach computeKnownBits/ComputeNumSignBits about atomics"
    
    This reverts commit 6c80361b8474535852afb2f7201370fb5f410091.
    Breaks PowerPC Big Endian buildbots.

I will keep you up to date as we have more information.
I have reopened this review.

I have access to big-endian PowerPC hardware. Is the reproducer as simple as running the compiler-rt test suite?

To reproduce the issue I use a stage2 bootstrap build and then run ninja check-asan. Check test will not finish.

To add to this more info.
I can reproduce the issue locally with this command:

Asan-powerpc64-calls-Test --gtest_filter=AddressSanitizer.OOBRightTest

I will get:

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from AddressSanitizer
[ RUN      ] AddressSanitizer.OOBRightTest

and then nothing... the test won't finish the run.

I have narrowed it down to the compilation of libclang_rt.asan-powerpc64.a if I link in a different version of that library when I link Asan-powerpc64-calls-Test then everything works as expected.

It seems that this patch has exposed an issue in the PowerPC backend where the partword atomics were not actually guaranteed to be zero extended.
I've added a patch:
https://reviews.llvm.org/D102819
to fix the issue.

The fix has been committed.
You may now try to recommit this patch.

Thank you for your patience!

The fix has been committed.
You may now try to recommit this patch.

Thank you for your patience!

Thanks; have you been able to verify the tests now pass on PPC64, or is that a "see what happens with the buildbots"? And will I need to update any of the tests you added when re-committing?

This revision was automatically updated to reflect the committed changes.