[SelectionDAG] Fix up SimplifyDemandedBits for ppcf128

[nemanjai]

Summary

On little endian PPC, the order of the doubles in a PPC double-double are reversed. When performing a "demanded bits" simplification, the demanded bits have to be rotated so that the bits come from the correct double.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=45475

[echristo]

Oof. That's painful, but I guess works for now.

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

852

Won't this rotate the bits every time you recursively call the function? You might need to limit this to Depth == 0 and then handle/bailout in the ISD::BITCAST case below.

[nemanjai]

Add handling for recursively traversing multiple nodes that produce ppcf128 - thanks for spotting this @RKSimon.
Also added a test case for which we perform such a traversal: (select (build_pair)).

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

852

Ah, this is a very good point. I think it would be very difficult to construct a test case where we traverse multiple nodes that produce a ppcf128 value, but it should be possible in principle. Limiting it to Depth == 0 would not achieve the desired result since we are likely entering the recursion from a different node (similarly to how we do so in the attached test case).

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

855

This is going to have the issue that it doesn't reset the flag to its old state when you leave the function, and TLO by design needs to be passed by reference.

Initially we might just have to bailout if we encounter MVT::ppcf128 until we can come up with a better solution?

[nemanjai]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

855

I took a look at the calls to this function and was fairly certain that every time we do a non-recursive call to this, we will create a new TLO (which would set the flag to its initial state of false). However, maybe that is not the case (or not guaranteed to be the case in the future).

My initial thoughts on this were to reset the flag to false upon entering recursion (i.e. Depth == 0) but I ended up not adding that for the above reason.

I believe that if I add something like the following above this condition:

if (Depth == 0)
  TLO.BitsRotatedForPPCF128 = false;

we will achieve the desired result. Leaving the bit set after leaving the function isn't really an issue - as long as it is reset when we enter the recursive call chain.

Conversely, I can add another parameter to the function with a default value of false to signal that we have encountered a ppcf128 and rotated the demanded bits. But I would favour the first approach I outlined above as it does not further complicate the API here.

[nemanjai]

@RKSimon What do you think about the above suggestion (i.e. clearing the flag at the start of recursion)?

[nemanjai]

Ping.

[nemanjai]

@RKSimon Do you think you'll have a chance to have another look at this some time soon?

[RKSimon]

oops - sorry I didn't see this as phab still puts it under "Waiting on Authors" - I'll take a look soon I promise!

[RKSimon]

please pre-commit the pr45475.ll test case against trunk to show the current codegen and rebase the patch to show the codegen delta

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

855

I'm still worried that we're not correctly resetting the flag when we return from deeper SimplifyDemandedBits calls (at Depth > 0 - not a new instance) - if we then have another f128 node that we attempt to recurse into as part of the same SimplifyDemandedBits call tree then the demandedbits that time won't get flipped - e.g. select (i1, f128, f128)?

llvm/test/CodeGen/PowerPC/pr45475.ll

42

do you need the dso_local ?

llvm/include/llvm/CodeGen/TargetLowering.h

public:
 
   /// A convenience struct that encapsulates a DAG, and two SDValues for
   /// returning information from TargetLowering to its clients that want to
   /// combine.
   struct TargetLoweringOpt {
     SelectionDAG &DAG;
     bool LegalTys;
     bool LegalOps;
+    bool BitsRotatedForPPCF128;
     SDValue Old;
     SDValue New;
 
     explicit TargetLoweringOpt(SelectionDAG &InDAG,
                                bool LT, bool LO) :
-      DAG(InDAG), LegalTys(LT), LegalOps(LO) {}
+      DAG(InDAG), LegalTys(LT), LegalOps(LO),
+      BitsRotatedForPPCF128(false) {}
 
     bool LegalTypes() const { return LegalTys; }
     bool LegalOperations() const { return LegalOps; }
 
     bool CombineTo(SDValue O, SDValue N) {
       Old = O;
       New = N;
       return true;

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

if (Op.getOpcode() == ISD::Constant) {
     // We know all of the bits for a constant!
     Known.One = cast<ConstantSDNode>(Op)->getAPIntValue();
     Known.Zero = ~Known.One;
     return false;
   }
 
   // Other users may use these bits.
   EVT VT = Op.getValueType();
+
+  // On little endian, the order of ppcf128 is backwards. However, if we
+  // recursively traverse multiple nodes that produce ppcf128 results, we
+  // don't want to keep rotating the bits.
+  if (VT == MVT::ppcf128 && TLO.DAG.getDataLayout().isLittleEndian() &&
+      !TLO.BitsRotatedForPPCF128) {
+    TLO.BitsRotatedForPPCF128 = true;
+    DemandedBits = DemandedBits.rotl(64);
+  }
+
   if (!Op.getNode()->hasOneUse() && !AssumeSingleUse) {
     if (Depth != 0) {
       // If not at the root, Just compute the Known bits to
       // simplify things downstream.
       Known = TLO.DAG.computeKnownBits(Op, DemandedElts, Depth);
       return false;
     }
     // If this is the root being simplified, allow it to have multiple uses,

llvm/test/CodeGen/PowerPC/pr45475.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-unknown \
+; RUN:     -mcpu=pwr8 -ppc-asm-full-reg-names < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-unknown \
+; RUN:     -mcpu=pwr8 -ppc-asm-full-reg-names < %s | FileCheck %s \
+; RUN:     --check-prefix=CHECK-BE
+define zeroext i1 @ppc128hi(ppc_fp128 %fp) {
+; CHECK-LABEL: ppc128hi:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    mffprd r3, f2
+; CHECK-NEXT:    rldicl r3, r3, 1, 63
+; CHECK-NEXT:    blr
+;
+; CHECK-BE-LABEL: ppc128hi:
+; CHECK-BE:       # %bb.0:
+; CHECK-BE-NEXT:    mffprd r3, f1
+; CHECK-BE-NEXT:    rldicl r3, r3, 1, 63
+; CHECK-BE-NEXT:    blr
+  %v128 = bitcast ppc_fp128 %fp to i128
+  %shift = lshr i128 %v128, 64
+  %high = trunc i128 %shift to i64
+  %c = icmp slt i64 %high, 0
+  ret i1 %c
+}
+define zeroext i1 @ppc128lo(ppc_fp128 %fp) {
+; CHECK-LABEL: ppc128lo:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    mffprd r3, f1
+; CHECK-NEXT:    rldicl r3, r3, 1, 63
+; CHECK-NEXT:    blr
+;
+; CHECK-BE-LABEL: ppc128lo:
+; CHECK-BE:       # %bb.0:
+; CHECK-BE-NEXT:    mffprd r3, f2
+; CHECK-BE-NEXT:    rldicl r3, r3, 1, 63
+; CHECK-BE-NEXT:    blr
+  %v128 = bitcast ppc_fp128 %fp to i128
+  %low = trunc i128 %v128 to i64
+  %c = icmp slt i64 %low, 0
+  ret i1 %c
+}
+define dso_local signext i32 @testMultNodes(ppc_fp128 %a, ppc_fp128 %b, i32 signext %c, i32 signext %d) {
+; CHECK-LABEL: testMultNodes:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    cmpw r7, r8
+; CHECK-NEXT:    bgt cr0, .LBB2_2
+; CHECK-NEXT:  # %bb.1: # %entry
+; CHECK-NEXT:    fmr f4, f2
+; CHECK-NEXT:  .LBB2_2: # %entry
+; CHECK-NEXT:    mffprd r3, f4
+; CHECK-NEXT:    rldicl r3, r3, 1, 63
+; CHECK-NEXT:    blr
+;
+; CHECK-BE-LABEL: testMultNodes:
+; CHECK-BE:       # %bb.0: # %entry
+; CHECK-BE-NEXT:    cmpw r7, r8
+; CHECK-BE-NEXT:    bgt cr0, .LBB2_2
+; CHECK-BE-NEXT:  # %bb.1: # %entry
+; CHECK-BE-NEXT:    fmr f3, f1
+; CHECK-BE-NEXT:  .LBB2_2: # %entry
+; CHECK-BE-NEXT:    mffprd r3, f3
+; CHECK-BE-NEXT:    rldicl r3, r3, 1, 63
+; CHECK-BE-NEXT:    blr
+entry:
+  %cmp = icmp sgt i32 %c, %d
+  %cond = select i1 %cmp, ppc_fp128 %b, ppc_fp128 %a
+  %0 = bitcast ppc_fp128 %cond to i128
+  %shr = lshr i128 %0, 127
+  %conv = trunc i128 %shr to i32
+  ret i32 %conv
+}