[PATCH] D18108: Swift Calling Convention: SelectionDAGBuilder changes to support swifterror

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Manman,

With mostly LGTM with a few nitpicks and clarification questions.

Cheers,
-Quentin


================
Comment at: include/llvm/CodeGen/FunctionLoweringInfo.h:84
@@ +83,3 @@
+  /// block. Entries in SwiftErrorVRegs have the same ordering as entries
+  /// in SwiftErrorValues.
+  /// Note that another choice that is more straight-forward is to use
----------------
SwiftErrorVals

================
Comment at: include/llvm/CodeGen/FunctionLoweringInfo.h:89
@@ +88,3 @@
+  /// machine basic block. This choice does not require a one-to-one
+  /// corresponse between SwiftErrorValues and SwiftErrorVRegs. But because of
+  /// efficiency concern, we do not choose it.
----------------
correspondence?

================
Comment at: include/llvm/CodeGen/FunctionLoweringInfo.h:95
@@ +94,3 @@
+  /// block when the basic block is not yet visited.
+  llvm::DenseMap<const MachineBasicBlock*, SwiftErrorVRegs>
+      SwiftErrorWorklist;
----------------
I found the comment confusing because the type in the mapping and the comment do not exactly match.
In particular, if we track a value at the end of a basic block:
1. There should be only one, i.e., we do not need a list-like type, do we?
2. If this is a value, why do we track a vreg, i.e., should the comment says vreg instead of value?

================
Comment at: include/llvm/CodeGen/FunctionLoweringInfo.h:100
@@ +99,3 @@
+  /// failure when the value does not exist in swifterror map.
+  unsigned findSwiftErrorVReg(const MachineBasicBlock*, const Value*) const;
+  /// Set the swifterror virtual register in SwiftErrorMap.
----------------
You could return 0 (noreg) as well. But I guess you do not want to spread the checks around (though you could assert on the result)

================
Comment at: include/llvm/Target/TargetLowering.h:2281
@@ +2280,3 @@
+  /// Return true if the target supports swifterror attribute.
+  virtual bool supportSwiftError() const {
+    return false;
----------------
I haven’t followed the other patches for this feature, but, assuming you have already explained what it takes to support that feature somewhere else, it would not hurt to sketch what it takes to support that feature.

================
Comment at: lib/CodeGen/SelectionDAG/FastISel.cpp:1328
@@ +1327,3 @@
+// Return true if we should copy from swift error to the final vreg as specified
+// by SwiftErrorWorklist.
+static bool shouldCopySwiftErrorsToFinalVRegs(const TargetLowering &TLI,
----------------
This function makes me wonder what happens if a function has the siwfterror attribute whereas the target does not support it?

================
Comment at: lib/CodeGen/SelectionDAG/FastISel.cpp:1361
@@ -1351,1 +1360,3 @@
+    if (shouldCopySwiftErrorsToFinalVRegs(TLI, FuncInfo))
+      return false;
     if (!handlePHINodesInSuccessorBlocks(I->getParent())) {
----------------
Could you add a comment on why it is ok not to handle the phi nodes in that case?
Also, say in the comment that we materialize the vreg in from worklist.

================
Comment at: lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp:608
@@ +607,3 @@
+      break;
+    }
+  assert(Index < End && "Can't find value in SwiftErrorVals");
----------------
std::find.

================
Comment at: lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp:622
@@ +621,3 @@
+      break;
+    }
+  assert(Index < End && "Can't find value in SwiftErrorVals");
----------------
Ditto.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:931
@@ +930,3 @@
+  auto &WorklistEntry = SDB.FuncInfo.SwiftErrorWorklist[SDB.FuncInfo.MBB];
+  auto &MapEntry = SDB.FuncInfo.SwiftErrorMap[SDB.FuncInfo.MBB];
+  for (unsigned I = 0, E = WorklistEntry.size(); I < E; I++) {
----------------
I am not a fan of auto when the type is not obvious. Could you add the explicit types here?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1465
@@ +1464,3 @@
+  if (F->getAttributes().hasAttrSomewhere(Attribute::SwiftError) &&
+      TLI.supportSwiftError()) {
+    ISD::ArgFlagsTy Flags = ISD::ArgFlagsTy();
----------------
Shouldn’t it be cheaper to check for the support of swift error first.
I don’t know how expensive it is to look “somewhere”, but that does not sound cheap :P.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3339
@@ -3285,1 +3338,3 @@
   const Value *SV = I.getOperand(0);
+  if (const Argument *Arg = dyn_cast<Argument>(SV)) {
+    const TargetLowering &TLI = DAG.getTargetLoweringInfo();
----------------
Guard those two with supportSwiftError, so that we do not have to go through the cast, get the TLI, and check the attribute for nothing.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3339
@@ -3285,1 +3338,3 @@
   const Value *SV = I.getOperand(0);
+  if (const Argument *Arg = dyn_cast<Argument>(SV)) {
+    const TargetLowering &TLI = DAG.getTargetLoweringInfo();
----------------
qcolombet wrote:
> Guard those two with supportSwiftError, so that we do not have to go through the cast, get the TLI, and check the attribute for nothing.
Also add a comment that swift error may arise from function parameter (Argument case) or inlining (alloca case), if I understand correctly.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3461
@@ +3460,3 @@
+                  SrcV->getType(), ValueVTs, &Offsets);
+  assert(ValueVTs.size() == 1 && "expect a single EVT for swifterror");
+
----------------
Also assert that Offsets[0] == 0, right?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3498
@@ +3497,3 @@
+  assert(ValueVTs.size() == 1 && "expect a single EVT for swifterror");
+
+  // Chain, DL, Reg, VT, Glue or Chain, DL, Reg, VT
----------------
Ditto.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3514
@@ -3392,1 +3513,3 @@
 
+  if (const Argument *Arg = dyn_cast<Argument>(PtrV)) {
+    const TargetLowering &TLI = DAG.getTargetLoweringInfo();
----------------
Same comments as the load counterpart.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5685
@@ +5684,3 @@
+      HasSwiftError = true;
+      Entry.Node = DAG.getRegister(FuncInfo.SwiftErrorMap[FuncInfo.MBB][0],
+                                   EVT(TLI.getPointerTy(DL)));
----------------
Add a comment repeating the assumption that if a function has a swift error argument, it must be the first in the list.
By the way, I am not even convinced it apply here. Why does this arguments would come form [0]?
Shouldn't we use findSwiftErrorVReg from V?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1244
@@ +1243,3 @@
+        unsigned VReg = FuncInfo->MF->getRegInfo().createVirtualRegister(RC);
+        FuncInfo->SwiftErrorWorklist[PredMBB].push_back(VReg);
+      }
----------------
Add a comment saying that those registers will be materialized when processing PredMBB and point to copySwiftErrorsToFinalVRegs.


http://reviews.llvm.org/D18108