Refactor: Simplify boolean conditional return statements in llvm/lib/CodeGen/SelectionDAG

[LegalizeAdulthood]

Summary

Use clang-tidy to simplify boolean conditional return statements

[craig.topper]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

9806–9807

Push the negate through and use an OR

9910

Use ==

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

2317

Push the negate through

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

2701–2702

Push the negate through

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

1364–1365

Push the negate through

[LegalizeAdulthood]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

9806–9807

Fixed.

9910

Fixed.

What's interesting here is that the tool already pushes through !(e1 != e2) to e1 == e2 when it is a simple == comparison. So I dug into this one to find out why it hadn't done that and it's because this is an operator== applied to an abstract data type. Because it is possible that operator!= has been overridden, but operator== has not been overridden, I didn't want to make the tool blindly push through on this kind of change. I'll need to figure out how to determine if operator== has been appropriately overridden before pushing through the simplification safely.

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

2317

Fixed.

What's a good guideline for the tool here?

I was thinking that it should apply DeMorgan's Theorem for the cases !(!e1 && !e2 && ... && !en) and !(!e1 || !e2 || ... || !en). In that situation it seems like a "no brainer" because it's the classic double negative on every term in the expression.

Here though, we have one of the inner terms negated and one not negated, so there's no clear simplification.

Is it always desirable to push through the outer negate whenever any of the inner terms also has a negation? I can see this being a matter of preference and therefore perhaps tweakable through a configuration option for the tool.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

2701–2702

Fixed.

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

1364–1365

Fixed.

[LegalizeAdulthood]

Update from comments, run clang-format on changes

[LegalizeAdulthood]

clang-format

[filcab]

lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp

2338

Would something like

return (int)SPQ->getCurCycle() < Height ||
       SPQ->getHazardRec()->getHazardType(SU, 0) !=
           ScheduleHazardRecognizer::NoHazard;

Be more readable, here? There are only two conditions.

2573

Same comment as for BUHasStall

[LegalizeAdulthood]

lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp

2338

To be clear: I'm just running clang-tidy on the code and submitting the diffs. None of these changes were created by hand.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

bool DAGCombiner::isSetCCEquivalent(SDValue N, SDValue &LHS, SDValue &RHS,
   return true;
 }
 
 /// Return true if this is a SetCC-equivalent operation with only one use.
 /// If this is true, it allows the users to invert the operation for free when
 /// it is profitable to do so.
 bool DAGCombiner::isOneUseSetCC(SDValue N) const {
   SDValue N0, N1, N2;
-  if (isSetCCEquivalent(N, N0, N1, N2) && N.getNode()->hasOneUse())
-    return true;
-  return false;
+  return isSetCCEquivalent(N, N0, N1, N2) && N.getNode()->hasOneUse();
 }
 
 /// Returns true if N is a BUILD_VECTOR node whose
 /// elements are all the same constant or undefined.
 static bool isConstantSplatVector(SDNode *N, APInt& SplatValue) {
   BuildVectorSDNode *C = dyn_cast<BuildVectorSDNode>(N);
   if (!C)
     return false;
if (!TLI.isLegalAddImmediate(getOffsetFromBase()))
       return false;
 
     // 3. Check that the computation is legal.
     if (!TLI.isOperationLegal(ISD::ADD, PtrType))
       return false;
 
     // Check that the zext is legal if it needs one.
     EVT TruncateType = Inst->getValueType(0);
-    if (TruncateType != SliceType &&
-        !TLI.isOperationLegal(ISD::ZERO_EXTEND, TruncateType))
-      return false;
-
-    return true;
+    return TruncateType == SliceType ||
+           TLI.isOperationLegal(ISD::ZERO_EXTEND, TruncateType);
   }
 
   /// \brief Get the offset in bytes of this slice in the original chunk of
   /// bits.
   /// \pre DAG != nullptr.
   uint64_t getOffsetFromBase() const {
     assert(DAG && "Missing context.");
     bool IsBigEndian =
bool canMergeExpensiveCrossRegisterBankCopy() const {
     if (RequiredAlignment > getAlignment())
       return false;
 
     // 2. Check that the load is a legal operation for that type.
     if (!TLI.isOperationLegal(ISD::LOAD, ResVT))
       return false;
 
     // 3. Check that we do not have a zext in the way.
-    if (Inst->getValueType(0) != getLoadedType())
-      return false;
-
-    return true;
+    return Inst->getValueType(0) == getLoadedType();
   }
 };
 }
 
 /// \brief Check that all bits set in \p UsedBits form a dense region, i.e.,
 /// \p UsedBits looks like 0..0 1..1 0..0.
 static bool areUsedBitsDense(const APInt &UsedBits) {
   // If all the bits are one, this is dense!

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

 /// and cos.
 static bool canCombineSinCosLibcall(SDNode *Node, const TargetLowering &TLI,
                                     const TargetMachine &TM) {
   if (!isSinCosLibcallAvailable(Node, TLI))
     return false;
   // GNU sin/cos functions set errno while sincos does not. Therefore
   // combining sin and cos is only safe if unsafe-fpmath is enabled.
   bool isGNU = Triple(TM.getTargetTriple()).getEnvironment() == Triple::GNU;
-  if (isGNU && !TM.Options.UnsafeFPMath)
-    return false;
-  return true;
+  return !isGNU || TM.Options.UnsafeFPMath;
 }
 
 /// Only issue sincos libcall if both sin and cos are needed.
 static bool useSinCos(SDNode *Node) {
   unsigned OtherOpcode = Node->getOpcode() == ISD::FSIN
     ? ISD::FCOS : ISD::FSIN;
 
   SDValue Op0 = Node->getOperand(0);

lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp

static bool hasVRegCycleUse(const SUnit *SU) {
   }
   return false;
 }
 
 // Check for either a dependence (latency) or resource (hazard) stall.
 //
 // Note: The ScheduleHazardRecognizer interface requires a non-const SU.
 static bool BUHasStall(SUnit *SU, int Height, RegReductionPQBase *SPQ) {
-  if ((int)SPQ->getCurCycle() < Height) return true;
-  if (SPQ->getHazardRec()->getHazardType(SU, 0)
-      != ScheduleHazardRecognizer::NoHazard)
+  if ((int)SPQ->getCurCycle() < Height)
     return true;
-  return false;
+  return SPQ->getHazardRec()->getHazardType(SU, 0) !=
+         ScheduleHazardRecognizer::NoHazard;
 }
 
 // Return -1 if left has higher priority, 1 if right has higher priority.
 // Return 0 if latency-based priority is equivalent.
 static int BUCompareLatency(SUnit *left, SUnit *right, bool checkPref,
                             RegReductionPQBase *SPQ) {
   // Scheduling an instruction that uses a VReg whose postincrement has not yet
   // been scheduled will induce a copy. Model this as an extra cycle of latency.

 // speed up processing the available queue.
 bool hybrid_ls_rr_sort::isReady(SUnit *SU, unsigned CurCycle) const {
   static const unsigned ReadyDelay = 3;
 
   if (SPQ->MayReduceRegPressure(SU)) return true;
 
   if (SU->getHeight() > (CurCycle + ReadyDelay)) return false;
 
-  if (SPQ->getHazardRec()->getHazardType(SU, -ReadyDelay)
-      != ScheduleHazardRecognizer::NoHazard)
-    return false;
-
-  return true;
+  return SPQ->getHazardRec()->getHazardType(SU, -ReadyDelay) ==
+         ScheduleHazardRecognizer::NoHazard;
 }
 
 // Return true if right should be scheduled with higher priority than left.
 bool hybrid_ls_rr_sort::operator()(SUnit *left, SUnit *right) const {
   if (int res = checkSpecialNodes(left, right))
     return res > 0;
 
   if (left->isCall || right->isCall)
bool hybrid_ls_rr_sort::operator()(SUnit *left, SUnit *right) const {
   return BURRSort(left, right, SPQ);
 }
 
 // Schedule as many instructions in each cycle as possible. So don't make an
 // instruction available unless it is ready in the current cycle.
 bool ilp_ls_rr_sort::isReady(SUnit *SU, unsigned CurCycle) const {
   if (SU->getHeight() > CurCycle) return false;
 
-  if (SPQ->getHazardRec()->getHazardType(SU, 0)
-      != ScheduleHazardRecognizer::NoHazard)
-    return false;
-
-  return true;
+  return SPQ->getHazardRec()->getHazardType(SU, 0) ==
+         ScheduleHazardRecognizer::NoHazard;
 }
 
 static bool canEnableCoalescing(SUnit *SU) {
   unsigned Opc = SU->getNode() ? SU->getNode()->getOpcode() : 0;
   if (Opc == ISD::TokenFactor || Opc == ISD::CopyToReg)
     // CopyToReg should be close to its uses to facilitate coalescing and
     // avoid spilling.
     return true;

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

for (unsigned i = 0, e = N->getNumOperands(); i < e; ++i) {
     } else if (ConstantFPSDNode *CFPN = dyn_cast<ConstantFPSDNode>(Zero)) {
       if (CFPN->getValueAPF().bitcastToAPInt().countTrailingZeros() < EltSize)
         return false;
     } else
       return false;
   }
 
   // Do not accept an all-undef vector.
-  if (IsAllUndef)
-    return false;
-  return true;
+  return !IsAllUndef;
 }
 
 /// \brief Return true if the specified node is a BUILD_VECTOR node of
 /// all ConstantSDNode or undef.
 bool ISD::isBuildVectorOfConstantSDNodes(const SDNode *N) {
   if (N->getOpcode() != ISD::BUILD_VECTOR)
     return false;
 

 /// ISD::ADD with a ConstantSDNode on the right-hand side, or if it is an
 /// ISD::OR with a ConstantSDNode that is guaranteed to have the same
 /// semantics as an ADD.  This handles the equivalence:
 ///     X|Cst == X+Cst iff X&Cst = 0.
 bool SelectionDAG::isBaseWithConstantOffset(SDValue Op) const {
   if ((Op.getOpcode() != ISD::ADD && Op.getOpcode() != ISD::OR) ||
       !isa<ConstantSDNode>(Op.getOperand(1)))
     return false;
 
-  if (Op.getOpcode() == ISD::OR &&
-      !MaskedValueIsZero(Op.getOperand(0),
-                     cast<ConstantSDNode>(Op.getOperand(1))->getAPIntValue()))
-    return false;
-
-  return true;
+  return Op.getOpcode() != ISD::OR ||
+         MaskedValueIsZero(
+             Op.getOperand(0),
+             cast<ConstantSDNode>(Op.getOperand(1))->getAPIntValue());
 }
 
-
 bool SelectionDAG::isKnownNeverNaN(SDValue Op) const {
   // If we're told that NaNs won't happen, assume they won't.
   if (getTarget().Options.NoNaNsFPMath)
     return true;
 
   // If the value is a constant, we can obviously see if it is a NaN or not.
   if (const ConstantFPSDNode *C = dyn_cast<ConstantFPSDNode>(Op))
     return !C->getValueAPF().isNaN();

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

static bool MIIsInTerminatorSequence(const MachineInstr *MI) {
 
   // Grab the copy source...
   MachineInstr::const_mop_iterator OPI2 = OPI;
   ++OPI2;
   assert(OPI2 != MI->operands_end()
          && "Should have a copy implying we should have 2 arguments.");
 
   // Make sure that the copy dest is not a vreg when the copy source is a
   // physical register.
-  if (!OPI2->isReg() ||
-      (!TargetRegisterInfo::isPhysicalRegister(OPI->getReg()) &&
-       TargetRegisterInfo::isPhysicalRegister(OPI2->getReg())))
-    return false;
-
-  return true;
+  return OPI2->isReg() &&
+         (TargetRegisterInfo::isPhysicalRegister(OPI->getReg()) ||
+          !TargetRegisterInfo::isPhysicalRegister(OPI2->getReg()));
 }
 
 /// Find the split point at which to splice the end of BB into its success stack
 /// protector check machine basic block.
 ///
 /// On many platforms, due to ABI constraints, terminators, even before register
 /// allocation, use physical registers. This creates an issue for us since
 /// physical registers at this point can not travel across basic

lib/CodeGen/SelectionDAG/TargetLowering.cpp

 
 bool
 TargetLowering::isOffsetFoldingLegal(const GlobalAddressSDNode *GA) const {
   // Assume that everything is safe in static mode.
   if (getTargetMachine().getRelocationModel() == Reloc::Static)
     return true;
 
   // In dynamic-no-pic mode, assume that known defined values are safe.
-  if (getTargetMachine().getRelocationModel() == Reloc::DynamicNoPIC &&
-      GA &&
+  return getTargetMachine().getRelocationModel() == Reloc::DynamicNoPIC && GA &&
          !GA->getGlobal()->isDeclaration() &&
-      !GA->getGlobal()->isWeakForLinker())
-    return true;
-
-  // Otherwise assume nothing is safe.
-  return false;
+         !GA->getGlobal()->isWeakForLinker();
 }
 
 //===----------------------------------------------------------------------===//
 //  Optimization Methods
 //===----------------------------------------------------------------------===//
 
 /// ShrinkDemandedConstant - Check to see if the specified operand of the
 /// specified instruction is a constant integer.  If so, check to see if there