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

[Manman Ren via llvm-commits]

manmanren added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/FastISel.cpp:1361
@@ -1351,1 +1360,3 @@
+    if (shouldCopySwiftErrorsToFinalVRegs(TLI, FuncInfo))
+      return false;
     if (!handlePHINodesInSuccessorBlocks(I->getParent())) {
----------------
qcolombet wrote:
> manmanren wrote:
> > If we need to materialize the vreg from worklist (i.e shouldCopySwiftErrorsToFinalVRegs returning true), we bail out of FastISel, so we should not need to call handlePHINodesInSuccessorBlocks?
> > 
> > Oops, forgot to update the comment.
> Ah make sense. Are you planning to update the comment?
Yes!

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5724
@@ +5723,3 @@
+      HasSwiftError = true;
+      // A function can only have a single swifterror argument. And if it does
+      // have a swifterror argument, it must be the first entry in
----------------
qcolombet wrote:
> manmanren wrote:
> > +  /// A function can only have a single swifterror argument. And if it does
> > +  /// have a swifterror argument, it must be the first entry in
> > +  /// SwiftErrorVals.
> > +  SwiftErrorValues SwiftErrorVals;
> > 
> > When setting up SwiftErrorVals, we always put the argument as the first entry, so findSwiftErrorVReg should return SwiftErrorMap[FuncInfo.MBB][0].
> Couldn’t we feed the argument with another swift error?
> E.g.,
> define void @foo(swift error %a)
>   %b = alloca swift error
>   call void @bar(%b) ; <— Here b is not in the first value in swifterrorvals, does it?
You are right, this is in LowerCall. The actual argument can come from an alloca.

Thanks for catching this, I will make the change.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5767
@@ +5766,3 @@
+    SDValue CopyNode = CLI.DAG.getCopyToReg(Result.second, CLI.DL, VReg, Src);
+    FuncInfo.SwiftErrorMap[FuncInfo.MBB][0] = VReg;
+    DAG.setRoot(CopyNode);
----------------
Same change here.


http://reviews.llvm.org/D18108