Obligatory monthly tail call patch

Project Infrastructure LLVM Dev List Archives
#1

Hello everybody, hi Evan,

this patch changes the lowering of arguments for tail call optimized
calls. Before arguments that could be overwritten by each other were
explicitly lowered to a stack slot, not giving the register allocator
a chance to optimize. Now a sequence of copyto/copyfrom virtual
registers ensures that arguments are loaded in (virtual) registers
before they are lowered to the stack slot (and might overwrite each
other). Also parameter stack slots are marked mutable for
(potentially) tail caling functions.

okay to commit?

regards
arnold

r47531-tc.patch (10.4 KB)

#2

Hi Arnold,

Thanks! Some comments.

+static bool IsPossiblyOverwrittenArgumentOfTailCall(SDOperand Op,
+ MachineFrameInfo * MFI) {
    RegisterSDNode * OpReg = NULL;
+ FrameIndexSDNode * FrameIdxNode = NULL;
+ int FrameIdx = 0;
    if (Op.getOpcode() == ISD::FORMAL_ARGUMENTS ||
        (Op.getOpcode()== ISD::CopyFromReg &&
- (OpReg = cast<RegisterSDNode>(Op.getOperand(1))) &&
- OpReg->getReg() >= TargetRegisterInfo::FirstVirtualRegister))
+ (OpReg = dyn_cast<RegisterSDNode>(Op.getOperand(1))) &&
+ (OpReg->getReg() >= TargetRegisterInfo::FirstVirtualRegister))

+ (Op.getOpcode() == ISD::LOAD &&
+ (FrameIdxNode = dyn_cast<FrameIndexSDNode>(Op.getOperand(1))) &&
+ ((FrameIdx = FrameIdxNode->getIndex()) < 0) &&
+ (MFI->getObjectOffset(FrameIdx) >= 0)))

Instead of assuming negative frame index has some special meaning, perhaps use isImmutableObjectIndex()?

+ int NewReturnAddrFI;
    if (IsTailCall) {
      // Adjust the Return address stack slot.
      if (FPDiff) {
@@ -1453,7 +1462,7 @@
          DAG.getLoad(VT, Chain,RetAddrFrIdx, NULL, 0);
        // Calculate the new stack slot for the return address.
        int SlotSize = Is64Bit ? 8 : 4;
- int NewReturnAddrFI =
+ NewReturnAddrFI =
          MF.getFrameInfo()->CreateFixedObject(SlotSize, FPDiff-SlotSize);
        NewRetAddrFrIdx = DAG.getFrameIndex(NewReturnAddrFI, VT);
        Chain = SDOperand(RetAddrFrIdx.Val, 1);
@@ -1461,13 +1470,13 @@
    }

...
      if (FPDiff)
+ Chain = DAG.getStore(Chain, RetAddrFrIdx, NewRetAddrFrIdx,
+ PseudoSourceValue::getFixedStack(), NewReturnAddrFI);

Looks like this may cause a compile time warning of using undefined value? How about moving the CreateFixedObject call down to the last if (FPDiff)?

+ SmallVector<std::pair<unsigned, SDOperand>, 8> EndangeredVRegs;

Picking on your choice of variable name here. :slight_smile: How about TailCallClobberedVRegs?

+ // Do not flag preceeding copytoreg stuff together with the following stuff.
+ InFlag = SDOperand();
+ // Create virtual register sources for all arguments to force arguments into
+ // (virtual) memory. To force (virtual) loading and guarantee that arguments
+ // sourcing from incomming parameters are not overwritten by each other.
+ for (unsigned i = 0, e = EndangeredVRegs.size(); i != e; i++) {
+ SDOperand Arg = EndangeredVRegs[i].second;
+ unsigned Idx = EndangeredVRegs[i].first;
+ unsigned VReg =
+ MF.getRegInfo().
+ createVirtualRegister(getRegClassFor(Arg.getValueType()));
+ Chain = DAG.getCopyToReg(Chain, VReg, Arg, InFlag);
+ InFlag = Chain.getValue(1);
+ Arg = DAG.getCopyFromReg(Chain, VReg, Arg.getValueType(), InFlag);
+ EndangeredVRegs[i] = std::make_pair(Idx, Arg);
+ Chain = Arg.getValue(1);
+ InFlag = Arg.getValue(2);
+ }

Please refactor this out to a static function.

Otherwise it looks great to me! Please commit after you've updated the patch.Thanks!

Evan