[LLVMdev] Obligatory monthly tail call patch

[Evan Cheng]

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. :-) 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


On Feb 23, 2008, at 1:01 PM, Arnold Schwaighofer wrote:

> 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>_______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev