[AArch64] ISel legalization debug messages. NFCI.

[SjoerdMeijer]

Summary

Debugging AArch64 instruction legalization and custom lowering is really an unpleasant experience because it shows nodes that appear out of thin air. This happens because newly created nodes in previous steps are not dumped, but are then picked up for legalization and printed.

This adds debug messages for several custom lowering functions; this is does not fix all custom lowerings, but is hopefully a good start.

[samparker]

Hi Sjoerd,

I completely agree that ISel can be a confusing pain with a lack of information! But wouldn't it be more helpful to make the detailed effort in SelectionDAG, in the calls to getNode, etc? Then all backends can take advantage and there isn't the burden on developers to remember to insert a call your debug function.

cheers,
sam

[SjoerdMeijer]

The disadvantage of moving it to SelectionDAG is that we lose the context where the new nodes are created. In other words, the debug message will (potentially) become less specific. But since they currently do not have an awful lot of context, I think your suggestion makes sense. I will experiment to see if it works.

[SjoerdMeijer]

I will split this patch up in a target dependent, and target independent patch. I will continue here with the AArch64 specific messages, and I have created D36984 for some of the debug messages that I moved to SelectionDAG getNode functions.

[SjoerdMeijer]

Removed the debug messages that were moved to SelectionDAG in D36984

[asb]

I think this is a really great initiative. I've added one inline comment. In addition to that, I have a general note. My natural instinct would have been to try to print a more informative message in the caller of the various is* functions (such as isLegalArithImmed). e.g. might better debug output be produced if debug printing were added to getAArch64Cmp instead?

lib/Target/AArch64/AArch64ISelLowering.cpp

4888 ↗

(On Diff #112170)

If we're going to print a debug message for every call, would it be worth at least dumping the global address symbol as well?

[SjoerdMeijer]

Hi Alex, thanks for the feedback. The difficulty here is finding the right balance of adding (good) debug messages, and not cluttering the code too much. I think what you are suggesting is what I had in my first revision: that had debug messages in the different AArch64 lowering functions. Then Sam's suggestion was to move some of these messages to SelectionDAG (which we did in D36984), which has 2 advantages: all targets benefit, and it does not clutter up the code. Also, I think this is quite difficult to get completely right the first time. So I think we will have to see what works and what is good and I will be adding more stuff as I go along. There's a lot of room for improvement here, but I already find these extra debug messages quite useful...

I have addressed your comment about the global address.

[asb]

In D36964#848739, @SjoerdMeijer wrote:

Hi Alex, thanks for the feedback. The difficulty here is finding the right balance of adding (good) debug messages, and not cluttering the code too much. I think what you are suggesting is what I had in my first revision: that had debug messages in the different AArch64 lowering functions. Then Sam's suggestion was to move some of these messages to SelectionDAG (which we did in D36984), which has 2 advantages: all targets benefit, and it does not clutter up the code. Also, I think this is quite difficult to get completely right the first time. So I think we will have to see what works and what is good and I will be adding more stuff as I go along. There's a lot of room for improvement here, but I already find these extra debug messages quite useful...

I definitely agree that this is something that can develop in time. I think splitting out some of the debug printing to the target-independent SelectionDAG code was a good move too. I suppose my comment only really applied to isLegalArithImmed. As you suggest, the other is* functions are called from outside AArch64ISelLowering anyway, and in the case of isFPImmLegal there's benefit in exposing details of that decision process. Anyway, I was interested if you'd thought about hoisting some of the debug messages to the caller, and the answer is obviously yes! This patch looks good to me, and I'm looking forward to monitoring how it this approach to ISel debugging develops.

[SjoerdMeijer]

Added missing spaces.

[samparker]

LGTM, thanks!

[SjoerdMeijer]

thanks for reviewing!

llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp

case ISD::SETUGE:
     Invert = true;
     changeFPCCToAArch64CC(getSetCCInverse(CC, false), CondCode, CondCode2);
     break;
   }
 }
 
 static bool isLegalArithImmed(uint64_t C) {
   // Matches AArch64DAGToDAGISel::SelectArithImmed().
-  return (C >> 12 == 0) || ((C & 0xFFFULL) == 0 && C >> 24 == 0);
+  bool IsLegal = (C >> 12 == 0) || ((C & 0xFFFULL) == 0 && C >> 24 == 0);
+  DEBUG(dbgs() << "Is imm " << C << " legal: " << (IsLegal ? "yes\n" : "no\n"));
+  return IsLegal;
 }
 
 static SDValue emitComparison(SDValue LHS, SDValue RHS, ISD::CondCode CC,
                               const SDLoc &dl, SelectionDAG &DAG) {
   EVT VT = LHS.getValueType();
   const bool FullFP16 =
     static_cast<const AArch64Subtarget &>(DAG.getSubtarget()).hasFullFP16();
 
SDValue AArch64TargetLowering::LowerINTRINSIC_WO_CHAIN(SDValue Op,
   case Intrinsic::aarch64_neon_umin:
     return DAG.getNode(ISD::UMIN, dl, Op.getValueType(),
                        Op.getOperand(1), Op.getOperand(2));
   }
 }
 
 SDValue AArch64TargetLowering::LowerOperation(SDValue Op,
                                               SelectionDAG &DAG) const {
+  DEBUG(dbgs() << "Custom lowering: ");
+  DEBUG(Op.dump());
+
   switch (Op.getOpcode()) {
   default:
     llvm_unreachable("unimplemented operand");
     return SDValue();
   case ISD::BITCAST:
     return LowerBITCAST(Op, DAG);
   case ISD::GlobalAddress:
     return LowerGlobalAddress(Op, DAG);
SDValue Lo = DAG.getNode(AArch64ISD::CSEL, dl, VT, LoForBigShift,
                            LoForNormalShift, CCVal, Cmp);
 
   SDValue Ops[2] = { Lo, Hi };
   return DAG.getMergeValues(Ops, dl);
 }
 
 bool AArch64TargetLowering::isOffsetFoldingLegal(
     const GlobalAddressSDNode *GA) const {
-  // The AArch64 target doesn't support folding offsets into global addresses.
+  DEBUG(dbgs() << "Skipping offset folding global address: ");
+  DEBUG(GA->dump());
+  DEBUG(dbgs() << "AArch64 doesn't support folding offsets into global "
+        "addresses\n");
   return false;
 }
 
 bool AArch64TargetLowering::isFPImmLegal(const APFloat &Imm, EVT VT) const {
   // We can materialize #0.0 as fmov $Rd, XZR for 64-bit and 32-bit cases.
   // FIXME: We should be able to handle f128 as well with a clever lowering.
-  if (Imm.isPosZero() && (VT == MVT::f64 || VT == MVT::f32))
+  if (Imm.isPosZero() && (VT == MVT::f64 || VT == MVT::f32)) {
+    DEBUG(dbgs() << "Legal fp imm: materialize 0 using the zero register\n");
     return true;
+  }
+
+  StringRef FPType;
+  bool IsLegal = false;
+  const StringRef Msg = "Is legal ";
+
+  if (VT == MVT::f64) {
+    FPType = "f64";
+    IsLegal = AArch64_AM::getFP64Imm(Imm) != -1;
+  } else if (VT == MVT::f32) {
+    FPType = "f32";
+    IsLegal = AArch64_AM::getFP32Imm(Imm) != -1;
+  } else if (VT == MVT::f16 && Subtarget->hasFullFP16()) {
+    FPType = "f16";
+    IsLegal = AArch64_AM::getFP16Imm(Imm) != -1;
+  }
+
+  if (IsLegal) {
+    DEBUG(dbgs() << Msg << FPType << " imm value: yes\n");
+    return true;
+  }
+
+  if (!FPType.empty())
+    DEBUG(dbgs() << Msg << FPType << " imm value: no\n");
+  else
+    DEBUG(dbgs() << Msg << "fp imm: no, unsupported fp type\n");
 
-  if (VT == MVT::f64)
-    return AArch64_AM::getFP64Imm(Imm) != -1;
-  else if (VT == MVT::f32)
-    return AArch64_AM::getFP32Imm(Imm) != -1;
-  else if (VT == MVT::f16 && Subtarget->hasFullFP16())
-    return AArch64_AM::getFP16Imm(Imm) != -1;
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 //                          AArch64 Optimization Hooks
 //===----------------------------------------------------------------------===//
 
 static SDValue getEstimate(const AArch64Subtarget *ST, unsigned Opcode,
if (Size >= 4 &&
        (allowsMisalignedMemoryAccesses(MVT::i32, 0, 1, &Fast) && Fast)))
     return MVT::i32;
 
   return MVT::Other;
 }
 
 // 12-bit optionally shifted immediates are legal for adds.
 bool AArch64TargetLowering::isLegalAddImmediate(int64_t Immed) const {
-  // Avoid UB for INT64_MIN.
-  if (Immed == std::numeric_limits<int64_t>::min())
+  if (Immed == std::numeric_limits<int64_t>::min()) {
+    DEBUG(dbgs() << "Illegal add imm " << Immed << ": avoid UB for INT64_MIN\n");
     return false;
+  }
   // Same encoding for add/sub, just flip the sign.
   Immed = std::abs(Immed);
-  return ((Immed >> 12) == 0 || ((Immed & 0xfff) == 0 && Immed >> 24 == 0));
+  bool IsLegal = ((Immed >> 12) == 0 ||
+                  ((Immed & 0xfff) == 0 && Immed >> 24 == 0));
+  DEBUG(dbgs() << "Is " << Immed << " legal add imm: " <<
+        (IsLegal ? "yes" : "no") << "\n");
+  return IsLegal;
 }
 
 // Integer comparisons are implemented with ADDS/SUBS, so the range of valid
 // immediates is the same as for an add or a sub.
 bool AArch64TargetLowering::isLegalICmpImmediate(int64_t Immed) const {
   return isLegalAddImmediate(Immed);
 }
 
static SDValue performNVCASTCombine(SDNode *N) {
   return SDValue();
 }
 
 SDValue AArch64TargetLowering::PerformDAGCombine(SDNode *N,
                                                  DAGCombinerInfo &DCI) const {
   SelectionDAG &DAG = DCI.DAG;
   switch (N->getOpcode()) {
   default:
+    DEBUG(dbgs() << "Custom combining: skipping\n");
     break;
   case ISD::ADD:
   case ISD::SUB:
     return performAddSubLongCombine(N, DCI, DAG);
   case ISD::XOR:
     return performXorCombine(N, DAG, DCI, Subtarget);
   case ISD::MUL:
     return performMulCombine(N, DAG, DCI, Subtarget);