[PPC] Lower llvm.ppc.cfence(constant) to no-op.

[timshen]

Summary

As the test case shows, if a pass constant-fold the generated
llvm.ppc.cfence(%tmp) to llvm.ppc.cfence(0), cfence should lower to
nothing, instead of crash later in legalization.

[kbarton]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

8220

I agree with this, but I'm wondering if it could/should be generalized a bit more.
Are there other cases of SDNodes that we could reach here that are not valid?
Alternatively, is it possible to enumerate the legal SD nodes that could be here and remove everything that is not legal?

llvm/test/CodeGen/PowerPC/atomics-constant.ll

4

The triple we normally use is powerpc64le-unknown-linux-gnu.

[timshen]

Rebase onto D33573, the crasher fix.

[timshen]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

8220

I dag a bit more and found the root cause, which is fixed in D33573. This patch, however only makes things faster, not more correct.

The optimization opportunity comes from that, a pass proves at compile time, that the load instruction itself doesn't synchronize with any stores, therefore it's Ok to change the parameter to @llvm.ppc.cfence to something else.

The interesting bits are "at compile time". This basically limits the optimization to ConstantSDNode only - other nodes represent behaviors at runtime, and are unlikely to be proven not synchronized with any stores whatsoever.

I suggest to keep these two cases (constant and others), until we find other pessimizations. Does it make sense?

[kbarton]

LGTM.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

8220

Yes, that's makes sense. Thanks for looking into this.

[timshen]

The problem is not PPC specific, so I'm going to abandon this patch, in order to not demotivate the correct fix. Someone should do it at IR level, when it actually pessimize their code. :)

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

 SDValue PPCTargetLowering::LowerINTRINSIC_VOID(SDValue Op,
                                                SelectionDAG &DAG) const {
   // SelectionDAGBuilder::visitTargetIntrinsic may insert one extra chain to
   // the beginning of the argument list.
   int ArgStart = isa<ConstantSDNode>(Op.getOperand(0)) ? 0 : 1;
   SDLoc DL(Op);
   switch (cast<ConstantSDNode>(Op.getOperand(ArgStart))->getZExtValue()) {
   case Intrinsic::ppc_cfence: {
+    assert(ArgStart == 1);
+    // If the operand is a constant (e.g. load from a constant pool being
+    // constant folded), there is no need to form a fence.
+    if (isa<ConstantSDNode>(Op.getOperand(ArgStart + 1))) {
+      return Op.getOperand(0);
+    }
     assert(Subtarget.isPPC64() && "Only 64-bit is supported for now.");
     return SDValue(DAG.getMachineNode(PPC::CFENCE8, DL, MVT::Other,
                                       DAG.getNode(ISD::ANY_EXTEND, DL, MVT::i64,
                                                   Op.getOperand(ArgStart + 1))),
                    0);
   }
   default:
     break;

llvm/test/CodeGen/PowerPC/atomics-constant.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s | FileCheck %s
+
+target triple = "powerpc64le-linux-gnu"
+
+@a = constant i64 zeroinitializer
+
+define i64 @foo() {
+; CHECK-LABEL: foo:
+; CHECK:       # BB#0: # %entry
+; CHECK-NEXT:    addis 3, 2, .LC0@toc@ha
+; CHECK-NEXT:    ld 3, .LC0@toc@l(3)
+; CHECK-NEXT:    ld 3, 0(3)
+; CHECK-NEXT:    li 3, 0
+; CHECK-NEXT:    blr
+entry:
+  %value = load atomic i64, i64* @a acquire, align 8
+  ret i64 %value
+}