Page MenuHomePhabricator

Swift Calling Convention: SelectionDAGBuilder changes to support swifterror
ClosedPublic

Authored by manmanren on Mar 11 2016, 3:52 PM.

Details

Summary

This patch is not as straight-forward as the first patch (reviews.llvm.org/D18092). It applies on top of the first patch for swifterror.
There is one more patch left for swifterror, the last patch will modify targets: ARM, X86 and AArch64.

At IR level, the swifterror argument is an input argument with type ErrorObject**. But we want to optimize it to behave as an extra return value with type ErrorObject*. It will be passed in a fixed physical register.

The main idea is to track the virtual registers for each swifterror value. We define swifterror values as AllocaInsts with swifterror attribute or a function argument with the attribute.

We introduce data structures in FunctionLoweringInfo to support versioning for swifterror values.
1> typedef SmallVector<const Value*, 1> SwiftErrorValues;

SwiftErrorValues SwiftErrorVals;
--> this is the vector to hold all swifterror values in the function.

2> typedef SmallVector<unsigned, 1> SwiftErrorVRegs;

llvm::DenseMap<const MachineBasicBlock*, SwiftErrorVRegs> SwiftErrorMap;
--> Track the virtual register for each swifterror value in a given basic block.

3> llvm::DenseMap<const MachineBasicBlock*, SwiftErrorVRegs> SwiftErrorWorklist;

--> Track the virtual register for a swifterror value at the end of a basic block when the basic block is not yet visited.

In SelectionDAGISel.cpp, we set up swifterror values (SwiftErrorVals) before handling the basic blocks.

When iterating over all basic blocks in RPO, before actually visiting the basic block, we call mergeIncomingSwiftErrors to merge incoming swifterror values when there are multiple predecessors or simply propagate them. There, we create a virtual register for each swifterror value in the entry block. For predecessors that are not yet visited, we create virtual registers to hold the swifterror values at the end of the predecessor. The assignments are saved in SwiftErrorWorklist.

At the end of a basic block, we copy swifterror values to the final virtual registers according to SwiftErrorWorklist.

When visiting a load from a swifterror value, we copy from the current virtual register assignment for the value. When visiting a store to a swifterror value, we create a virtual register to hold the swifterror value and update SwiftErrorMap to track the current virtual register assignment.

When handling the ReturnInst, we fake an output argument for the swifterror argument, its value coming from the virtual register assignment. Target-specific calling convention can assign the output argument to a certain physical register.

When lowering a call to a function with a swifterror parameter, we fake an entry in CLI.Ins (the last entry in CLI.Ins) with pointer type. Target-specific calling convention can assign the output argument to a certain physical register. We then get the last element of CLI.InVals and copy it to a newly-created virtual register and update SwiftErrorMap for version tracking.

Diff Detail

Repository
rL LLVM

Event Timeline

manmanren updated this revision to Diff 50494.Mar 11 2016, 3:52 PM
manmanren retitled this revision from to Swift Calling Convention: SelectionDAGBuilder changes to support swifterror.
manmanren updated this object.
manmanren added reviewers: rengolin, hfinkel, qcolombet.
manmanren added subscribers: llvm-commits, rjmccall.
qcolombet edited edge metadata.Apr 4 2016, 1:23 PM

Hi Manman,

With mostly LGTM with a few nitpicks and clarification questions.

Cheers,
-Quentin

include/llvm/CodeGen/FunctionLoweringInfo.h
84 ↗(On Diff #50494)

SwiftErrorVals

89 ↗(On Diff #50494)

correspondence?

95 ↗(On Diff #50494)

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?
100 ↗(On Diff #50494)

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)

include/llvm/Target/TargetLowering.h
2281 ↗(On Diff #50494)

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.

lib/CodeGen/SelectionDAG/FastISel.cpp
1328 ↗(On Diff #50494)

This function makes me wonder what happens if a function has the siwfterror attribute whereas the target does not support it?

1361 ↗(On Diff #50494)

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.

lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
608 ↗(On Diff #50494)

std::find.

622 ↗(On Diff #50494)

Ditto.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
931 ↗(On Diff #50494)

I am not a fan of auto when the type is not obvious. Could you add the explicit types here?

1465 ↗(On Diff #50494)

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.

3339 ↗(On Diff #50494)

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.

3339 ↗(On Diff #50494)

Also add a comment that swift error may arise from function parameter (Argument case) or inlining (alloca case), if I understand correctly.

3461 ↗(On Diff #50494)

Also assert that Offsets[0] == 0, right?

3498 ↗(On Diff #50494)

Ditto.

3514 ↗(On Diff #50494)

Same comments as the load counterpart.

5685 ↗(On Diff #50494)

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?

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
1244 ↗(On Diff #50494)

Add a comment saying that those registers will be materialized when processing PredMBB and point to copySwiftErrorsToFinalVRegs.

manmanren updated this revision to Diff 52638.Apr 4 2016, 4:14 PM
manmanren edited edge metadata.
manmanren marked 14 inline comments as done.Apr 4 2016, 4:29 PM

Hi Quentin,

Thanks for reviewing the patch. I uploaded a new version to address your comments.

Cheers,
Manman

lib/CodeGen/SelectionDAG/FastISel.cpp
1328 ↗(On Diff #52638)

For targets that support swifterror attribute, all accesses to swifterror values (with type swift_error_object) will be converted to use the corresponding virtual register for the swifterror value. For targets that do not support swifterror attribute, the attribute will be ignored. We will load and store swift_error_object.

1361 ↗(On Diff #52638)

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.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5724 ↗(On Diff #52638)

+ / 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].

qcolombet added inline comments.Apr 4 2016, 5:49 PM
lib/CodeGen/SelectionDAG/FastISel.cpp
1361 ↗(On Diff #52638)

Ah make sense. Are you planning to update the comment?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5724 ↗(On Diff #52638)

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?
manmanren added inline comments.Apr 4 2016, 10:32 PM
lib/CodeGen/SelectionDAG/FastISel.cpp
1361 ↗(On Diff #52638)

Yes!

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5724 ↗(On Diff #52638)

You are right, this is in LowerCall. The actual argument can come from an alloca.

Thanks for catching this, I will make the change.

5767 ↗(On Diff #52638)

Same change here.

manmanren updated this revision to Diff 52711.Apr 5 2016, 10:33 AM

Addressing review comments.

qcolombet accepted this revision.Apr 5 2016, 10:52 AM
qcolombet edited edge metadata.

Hi Manman,

LGTM with one comment.

Cheers,
-Quentin

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5724 ↗(On Diff #52711)

Could you add a test case for that change?

This revision is now accepted and ready to land.Apr 5 2016, 10:52 AM
manmanren marked 3 inline comments as done.Apr 5 2016, 11:00 AM

Thanks for reviewing the patch!
Manman

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5724 ↗(On Diff #52711)

Sure. I will upload a test case with the target-specific change (http://reviews.llvm.org/D18716).

This revision was automatically updated to reflect the committed changes.