[PowerPC] Fallback to base's implementation of shouldExpandAtomicCmpXchgInIR and shouldExpandAtomicCmpXchgInIR

[lkail]

Summary

If we can't decide shouldExpandAtomicCmpXchgInIR or shouldExpandAtomicCmpXchgInIR in PPC's implementation after https://reviews.llvm.org/rGb9c3941cd61de1e1b9e4f3311ddfa92394475f4b, resort to base's implementation.

This fixes internal build of OpenMP which uses atomic operations on float.

[lkail]

Add test for opt.

[jsji]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

17501

With this change, we pretty much fall back on base implementation for *ALL* other types , except for 128 bits. That means quite some change to existing expansion? If so, I think we should at least add tests for all other types and bits to double confirm.

[lkail]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

17501

Before https://reviews.llvm.org/rGb9c3941cd61de1e1b9e4f3311ddfa92394475f4b which override shouldExpandAtomicRMWInIR, calling shouldExpandAtomicRMWInIR will falls into TargetLoweringBase's

/// Returns how the IR-level AtomicExpand pass should expand the given
/// AtomicRMW, if at all. Default is to never expand.
virtual AtomicExpansionKind shouldExpandAtomicRMWInIR(AtomicRMWInst *RMW) const {
  return RMW->isFloatingPointOperation() ?
    AtomicExpansionKind::CmpXChg : AtomicExpansionKind::None;
}

After override and this patch, I think all other types remain what they are before https://reviews.llvm.org/rGb9c3941cd61de1e1b9e4f3311ddfa92394475f4b.

[lkail]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

17501

And indeed, we're lacking tests while performing AtomicExpand, like atomic float operations. I'll add some tests involving AtomicExpand as regression tests.

[dyung]

Hi, the test you added is causing a failure on the PS4 build bot, https://lab.llvm.org/buildbot/#/builders/139/builds/7433.

Can you take a look and please fix the test or revert the change so that we can get the bot green again?

[DavidSpickett]

One of these tests is failing on Arm bots: https://lab.llvm.org/buildbot/#/builders/171/builds/1162

Because we only build one specific target and this test dir doesn't require PowerPC. I'll fix that.

[lkail]

In D106234#2889770, @dyung wrote:

Hi, the test you added is causing a failure on the PS4 build bot, https://lab.llvm.org/buildbot/#/builders/139/builds/7433.

Can you take a look and please fix the test or revert the change so that we can get the bot green again?

I'm looking into it. Thanks for notification.

[DavidSpickett]

Now I posted the comment I see someone else reported it. I'll leave it up to you then to prevent any confusion.

[dyung]

In D106234#2889775, @DavidSpickett wrote:

Now I posted the comment I see someone else reported it. I'll leave it up to you then to prevent any confusion.

Our PS4 bot also only builds one target (x86_64), so it might be that the test requires a PPC target, and if so, should add that.

[lkail]

In D106234#2889772, @DavidSpickett wrote:

One of these tests is failing on Arm bots: https://lab.llvm.org/buildbot/#/builders/171/builds/1162

Because we only build one specific target and this test dir doesn't require PowerPC. I'll fix that.

https://github.com/llvm/llvm-project/commit/1453f048cf9275ac329b5beb243c3c0986144143 Done.

[DavidSpickett]

Thanks, bot is green again. https://lab.llvm.org/buildbot/#/builders/188/builds/1143

[dyung]

Thanks for the fix, our bot is not green, but at least now it isn't your fault!

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

case CallingConv::Cold:
     return (Return ? RetCC_PPC_Cold : CC_PPC64_ELF_FIS);
   default:
     return CC_PPC64_ELF_FIS;
   }
 }
 
 TargetLowering::AtomicExpansionKind
 PPCTargetLowering::shouldExpandAtomicRMWInIR(AtomicRMWInst *AI) const {
-  if (AI->isFloatingPointOperation())
-    return AtomicExpansionKind::None;
   unsigned Size = AI->getType()->getPrimitiveSizeInBits();
   if (EnableQuadwordAtomics && Subtarget.hasQuadwordAtomics() && Size == 128)
     return AtomicExpansionKind::MaskedIntrinsic;
-  return AtomicExpansionKind::None;
+  return TargetLowering::shouldExpandAtomicRMWInIR(AI);
 }
 
 TargetLowering::AtomicExpansionKind
 PPCTargetLowering::shouldExpandAtomicCmpXchgInIR(AtomicCmpXchgInst *AI) const {
   unsigned Size = AI->getPointerOperand()
                       ->getType()
                       ->getPointerElementType()
                       ->getPrimitiveSizeInBits();
   if (EnableQuadwordAtomics && Subtarget.hasQuadwordAtomics() && Size == 128)
     return AtomicExpansionKind::MaskedIntrinsic;
-  return AtomicExpansionKind::None;
+  return TargetLowering::shouldExpandAtomicCmpXchgInIR(AI);
 }
 
 static Intrinsic::ID
 getIntrinsicForAtomicRMWBinOp128(AtomicRMWInst::BinOp BinOp) {
   switch (BinOp) {
   default:
     llvm_unreachable("Unexpected AtomicRMW BinOp");
   case AtomicRMWInst::Xchg:

llvm/test/CodeGen/PowerPC/atomic-float.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-unknown \
+; RUN:   < %s | FileCheck --check-prefix=CHECK-64 %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc-unknown-unknown \
+; RUN:   < %s | FileCheck --check-prefix=CHECK-32 %s
+
+define float @test_add(float* %ptr, float %incr) {
+; CHECK-64-LABEL: test_add:
+; CHECK-64:       # %bb.0: # %entry
+; CHECK-64-NEXT:    sync
+; CHECK-64-NEXT:    lfs 0, 0(3)
+; CHECK-64-NEXT:    b .LBB0_3
+; CHECK-64-NEXT:  .LBB0_1: # %atomicrmw.start
+; CHECK-64-NEXT:    #
+; CHECK-64-NEXT:    stwcx. 5, 0, 3
+; CHECK-64-NEXT:  .LBB0_2: # %atomicrmw.start
+; CHECK-64-NEXT:    #
+; CHECK-64-NEXT:    stw 5, -4(1)
+; CHECK-64-NEXT:    cmplw 5, 4
+; CHECK-64-NEXT:    lfs 0, -4(1)
+; CHECK-64-NEXT:    beq 0, .LBB0_6
+; CHECK-64-NEXT:  .LBB0_3: # %atomicrmw.start
+; CHECK-64-NEXT:    # =>This Loop Header: Depth=1
+; CHECK-64-NEXT:    # Child Loop BB0_4 Depth 2
+; CHECK-64-NEXT:    fadds 2, 0, 1
+; CHECK-64-NEXT:    stfs 2, -8(1)
+; CHECK-64-NEXT:    stfs 0, -12(1)
+; CHECK-64-NEXT:    lwz 6, -8(1)
+; CHECK-64-NEXT:    lwz 4, -12(1)
+; CHECK-64-NEXT:  .LBB0_4: # %atomicrmw.start
+; CHECK-64-NEXT:    # Parent Loop BB0_3 Depth=1
+; CHECK-64-NEXT:    # => This Inner Loop Header: Depth=2
+; CHECK-64-NEXT:    lwarx 5, 0, 3
+; CHECK-64-NEXT:    cmpw 4, 5
+; CHECK-64-NEXT:    bne 0, .LBB0_1
+; CHECK-64-NEXT:  # %bb.5: # %atomicrmw.start
+; CHECK-64-NEXT:    #
+; CHECK-64-NEXT:    stwcx. 6, 0, 3
+; CHECK-64-NEXT:    bne 0, .LBB0_4
+; CHECK-64-NEXT:    b .LBB0_2
+; CHECK-64-NEXT:  .LBB0_6: # %atomicrmw.end
+; CHECK-64-NEXT:    fmr 1, 0
+; CHECK-64-NEXT:    lwsync
+; CHECK-64-NEXT:    blr
+;
+; CHECK-32-LABEL: test_add:
+; CHECK-32:       # %bb.0: # %entry
+; CHECK-32-NEXT:    stwu 1, -32(1)
+; CHECK-32-NEXT:    .cfi_def_cfa_offset 32
+; CHECK-32-NEXT:    sync
+; CHECK-32-NEXT:    lfs 0, 0(3)
+; CHECK-32-NEXT:    b .LBB0_3
+; CHECK-32-NEXT:  .LBB0_1: # %atomicrmw.start
+; CHECK-32-NEXT:    #
+; CHECK-32-NEXT:    stwcx. 5, 0, 3
+; CHECK-32-NEXT:  .LBB0_2: # %atomicrmw.start
+; CHECK-32-NEXT:    #
+; CHECK-32-NEXT:    stw 5, 28(1)
+; CHECK-32-NEXT:    cmplw 5, 4
+; CHECK-32-NEXT:    lfs 0, 28(1)
+; CHECK-32-NEXT:    beq 0, .LBB0_6
+; CHECK-32-NEXT:  .LBB0_3: # %atomicrmw.start
+; CHECK-32-NEXT:    # =>This Loop Header: Depth=1
+; CHECK-32-NEXT:    # Child Loop BB0_4 Depth 2
+; CHECK-32-NEXT:    fadds 2, 0, 1
+; CHECK-32-NEXT:    stfs 2, 24(1)
+; CHECK-32-NEXT:    stfs 0, 20(1)
+; CHECK-32-NEXT:    lwz 6, 24(1)
+; CHECK-32-NEXT:    lwz 4, 20(1)
+; CHECK-32-NEXT:  .LBB0_4: # %atomicrmw.start
+; CHECK-32-NEXT:    # Parent Loop BB0_3 Depth=1
+; CHECK-32-NEXT:    # => This Inner Loop Header: Depth=2
+; CHECK-32-NEXT:    lwarx 5, 0, 3
+; CHECK-32-NEXT:    cmpw 4, 5
+; CHECK-32-NEXT:    bne 0, .LBB0_1
+; CHECK-32-NEXT:  # %bb.5: # %atomicrmw.start
+; CHECK-32-NEXT:    #
+; CHECK-32-NEXT:    stwcx. 6, 0, 3
+; CHECK-32-NEXT:    bne 0, .LBB0_4
+; CHECK-32-NEXT:    b .LBB0_2
+; CHECK-32-NEXT:  .LBB0_6: # %atomicrmw.end
+; CHECK-32-NEXT:    fmr 1, 0
+; CHECK-32-NEXT:    lwsync
+; CHECK-32-NEXT:    addi 1, 1, 32
+; CHECK-32-NEXT:    blr
+entry:
+  %r = atomicrmw fadd float* %ptr, float %incr seq_cst
+  ret float %r
+}