Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/lib/Basic/Targets/SystemZ.h
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,8 @@ class LLVM_LIBRARY_VISIBILITY SystemZTargetInfo : public TargetInfo {
return RegNo < 4 ? 6 + RegNo : -1;
}

bool hasSjLjLowering() const override { return true; }

std::pair<unsigned, unsigned> hardwareInterferenceSizes() const override {
return std::make_pair(256, 256);
}
Expand Down
6 changes: 6 additions & 0 deletions clang/lib/CodeGen/CGBuiltin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4619,6 +4619,12 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
// Buffer is a void**.
Address Buf = EmitPointerWithAlignment(E->getArg(0));

if (getTarget().getTriple().getArch() == llvm::Triple::systemz) {
// Call LLVM's EH setjmp, which is lightweight.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment here should explain that we're not filling any fields of the jmp_buf here and leave it all to the back-end.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned about modifying the semantics of the intrinsic like this. I mean, the semantics are inherently a bit platform-specific, but this contradicts the documentation in llvm/docs/ExceptionHandling.rst.

If we need different semantics here for some reason, I'd prefer to use an intrinsic with a different name, to avoid any confusion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying problem is that current implementation and documentation of the builtin is somewhat inconsistent with itself. The documentation starts out with:

The buffer format and the overall functioning of this intrinsic is compatible with the GCC __builtin_setjmp implementation allowing code built with the clang and GCC to interoperate.

But in GCC the format of the buffer is completely target-dependent (and actually materially different between targets), while LLVM attempts to impose some constraints on certain fields. (These happen to be OK for the small subset of targets that currently implement these intrinsics in LLVM, but not all other GCC targets.)

Specifically, the LLVM documentation continues with

The front end places the frame pointer in the first word

Now, even in GCC every target places the frame pointer in the first word, but in GCC this means the contents of the target frame pointer register, if any is used. What clang puts into the first word, however, is the result of the frameaddress intrinsic - this value matches the frame pointer register contents on some but not all targets. On SystemZ these values differ, which would cause incompatibilties with GCC.

Finally, the clang front-end also writes the stack pointer into the third slot. Not only is this not even mentioned in the docs, it also does not match GCC behavior on all targets. While indeed all targets have the to save the stack pointer somewhere, they're not all using the third slot. On SystemZ we use the fourth slot.

It is a bit unclear to me what the purpose of writing those two slots in the front end is in the first place. It would seem more straightforward to just leave the writing of the buffer completely to the target back-end.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. That's pretty messy. See also 02269a6 and faa3abb . I think what happened is that the meaning of the intrinsic was adapted to suit sjlj lowering, and nobody really thought about trying to expose a stable interface to frontends.

I still think it's a bit of a hazard to adjust the semantics of the intrinsic at this point without changing the name. But there isn't any existing SystemZ bitcode to break, so it might be enough to just update the documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. It seems back in the day the eh.sjlj.setjmp intrinsic was used by both the front-end to implement __builtin_setjmp and by the middle-end (SjLjEHPrepare.cpp) for exception handling. Since these two users apparently require somewhat different information in the buffer, filling this in was left to the user.

However, these days those two users actually emit two different intrinsics. The middle-end now uses eh.sjlj.setup.dispatch (which is apparently only intended for internal usage; it's not documented at all), while only the front-end continues to use eh.sjlj.setjmp. See http://reviews.llvm.org/D9313.

I still think the most straightfoward way would be to have eh.sjlj.setjmp just be completely implemented in the back end, and simply do exactly what is needed to implement __builtin_setjmp on the platform, just like is done with longjmp. This could be done for all targets (probably the cleanest solution, but would require updating the existing targets and might introduce incompatibilities with existing bytecode on those targets), or just for selected targets (I guess this would then need to be documented? And should it be a positive or negative list of targets?).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I don't think there's any benefit to having the frontend to generate explicit stores. I just want to make sure whatever solution we choose, it doesn't silently break code generated by other frontends (or older versions of clang).

Function *F = CGM.getIntrinsic(Intrinsic::eh_sjlj_setjmp);
return RValue::get(Builder.CreateCall(F, Buf.emitRawPointer(*this)));
}

// Store the frame pointer to the setjmp buffer.
Value *FrameAddr = Builder.CreateCall(
CGM.getIntrinsic(Intrinsic::frameaddress, AllocaInt8PtrTy),
Expand Down
4 changes: 4 additions & 0 deletions llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,10 @@ void SystemZAsmPrinter::emitInstruction(const MachineInstr *MI) {
MCInstBuilder(SystemZ::EXRL).addReg(LenMinus1Reg).addExpr(Dot));
return;
}
// EH_SjLj_Setup is a dummy terminator instruction of size 0,
// It is used to handle the clobber register for builtin setjmp.
case SystemZ::EH_SjLj_Setup:
return;

default:
Lower.lower(MI, LoweredMI);
Expand Down
253 changes: 252 additions & 1 deletion llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,13 @@ SystemZTargetLowering::SystemZTargetLowering(const TargetMachine &TM,
setOperationAction(ISD::INTRINSIC_W_CHAIN, MVT::Other, Custom);
setOperationAction(ISD::INTRINSIC_WO_CHAIN, MVT::Other, Custom);

// We're not using SJLJ for exception handling, but they're implemented
// solely to support use of __builtin_setjmp / __builtin_longjmp.
setOperationAction(ISD::EH_SJLJ_SETJMP, MVT::i32, Custom);
setOperationAction(ISD::EH_SJLJ_LONGJMP, MVT::Other, Custom);


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't add those additional newlines. One empty line is enough.


// We want to use MVC in preference to even a single load/store pair.
MaxStoresPerMemcpy = Subtarget.hasVector() ? 2 : 0;
MaxStoresPerMemcpyOptSize = 0;
Expand Down Expand Up @@ -940,7 +947,242 @@ bool SystemZTargetLowering::isFPImmLegal(const APFloat &Imm, EVT VT,
return SystemZVectorConstantInfo(Imm).isVectorConstantLegal(Subtarget);
}

/// Returns true if stack probing through inline assembly is requested.

MachineBasicBlock *
SystemZTargetLowering::emitEHSjLjSetJmp(MachineInstr &MI,
MachineBasicBlock *MBB) const {

DebugLoc DL = MI.getDebugLoc();
const TargetInstrInfo *TII = Subtarget.getInstrInfo();
const SystemZRegisterInfo *TRI = Subtarget.getRegisterInfo();

MachineFunction *MF = MBB->getParent();
MachineRegisterInfo &MRI = MF->getRegInfo();

const BasicBlock *BB = MBB->getBasicBlock();
MachineFunction::iterator I = ++MBB->getIterator();

Register DstReg = MI.getOperand(0).getReg();
const TargetRegisterClass *RC = MRI.getRegClass(DstReg);
assert(TRI->isTypeLegalForClass(*RC, MVT::i32) && "Invalid destination!");
Register mainDstReg = MRI.createVirtualRegister(RC);
Register restoreDstReg = MRI.createVirtualRegister(RC);

MVT PVT = getPointerTy(MF->getDataLayout());
assert((PVT == MVT::i64 || PVT == MVT::i32) &&
"Invalid Pointer Size!");
// For v = setjmp(buf), we generate.
// Algorithm:
//
// ---------
// | thisMBB |
// ---------
// |
// ------------------------
// | |
// ---------- ---------------
// | mainMBB | | restoreMBB |
// | v = 0 | | v = 1 |
// ---------- ---------------
// | |
// -------------------------
// |
// -----------------------------
// | sinkMBB |
// | phi(v_mainMBB,v_restoreMBB) |
// -----------------------------
// thisMBB:
// buf[0] = Frame Pointer if hasFP.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use FPOffset here to be consistent with the code below.

// buf[LabelOffset] = restoreMBB <-- takes address of restoreMBB.
// buf[BCOffset] = Backchain value if building with -mbackchain.
// buf[SPOffset] = Stack Pointer.
// buf[LPOffset] = We never write this slot with R13, gcc stores R13 always.
// SjLjSetup restoreMBB
// mainMBB:
// v_main = 0
// sinkMBB:
// v = phi(v_main, v_restore)
// restoreMBB:
// v_restore = 1

MachineBasicBlock *thisMBB = MBB;
MachineBasicBlock *mainMBB = MF->CreateMachineBasicBlock(BB);
MachineBasicBlock *sinkMBB = MF->CreateMachineBasicBlock(BB);
MachineBasicBlock *restoreMBB = MF->CreateMachineBasicBlock(BB);

MF->insert(I, mainMBB);
MF->insert(I, sinkMBB);
MF->push_back(restoreMBB);
restoreMBB->setMachineBlockAddressTaken();

MachineInstrBuilder MIB;

// Transfer the remainder of BB and its successor edges to sinkMBB.
sinkMBB->splice(sinkMBB->begin(), MBB,
std::next(MachineBasicBlock::iterator(MI)), MBB->end());
sinkMBB->transferSuccessorsAndUpdatePHIs(MBB);


// thisMBB:
const int64_t LabelOffset = 1 * PVT.getStoreSize(); // Slot 2.
const int64_t SPOffset = 3 * PVT.getStoreSize(); // Slot 4.

// Buf address.
Register BufReg = MI.getOperand(1).getReg();

unsigned LabelReg = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the zero initialization here. Just move the declaration down.

const TargetRegisterClass *PtrRC = getRegClassFor(PVT);
LabelReg = MRI.createVirtualRegister(PtrRC);

// prepare IP for longjmp.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comments that form a sentence should start with a capital letter.

BuildMI(*thisMBB, MI, DL, TII->get(SystemZ::LARL), LabelReg)
.addMBB(restoreMBB);

// store IP for return from jmp, slot 2, offset = 1.
BuildMI(*thisMBB, MI, DL, TII->get(SystemZ::STG))
.addReg(LabelReg)
.addReg(BufReg)
.addImm(LabelOffset)
.addReg(0);

bool HasFP = Subtarget.getFrameLowering()->hasFP(*MF);
if (HasFP) {
const int64_t FPOffset = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be preferable to have all five Offset constant declaration in one place near the top of the function.

BuildMI(*thisMBB, MI, DL, TII->get(SystemZ::STG))
.addReg(SystemZ::R11D)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this function will probably only be used on Linux, we still should use getFramePointerRegister() (from Subtarget.getSpecialRegisters()) here instead of hardcoding R11.

.addReg(BufReg)
.addImm(FPOffset)
.addReg(0);
}

// store SP.
BuildMI(*thisMBB, MI, DL, TII->get(SystemZ::STG))
.addReg(SystemZ::R15D)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly getStackPointerRegister() here.

.addReg(BufReg)
.addImm(SPOffset)
.addReg(0);

// Slot 3(Offset = 2) Backchain value (if building with -mbackchain).
bool BackChain = MF->getSubtarget<SystemZSubtarget>().hasBackChain();
if (BackChain) {
const int64_t BCOffset = 2 * PVT.getStoreSize();
Register BCReg = MRI.createVirtualRegister(RC);
MIB = BuildMI(*thisMBB, MI, DL, TII->get(SystemZ::LG), BCReg)
.addReg(SystemZ::R15D)
.addImm(0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, both SP and the backchain offset should be taken from the OS backend, using something like getBackchainAddress. See the implementation in lowerSTACKRESTORE as example.

.addReg(0);

BuildMI(*thisMBB, MI, DL, TII->get(SystemZ::STG))
.addReg(BCReg)
.addReg(BufReg)
.addImm(BCOffset)
.addReg(0);
}

// Setup.
MIB = BuildMI(*thisMBB, MI, DL, TII->get(SystemZ::EH_SjLj_Setup))
.addMBB(restoreMBB);

const SystemZRegisterInfo *RegInfo = Subtarget.getRegisterInfo();
MIB.addRegMask(RegInfo->getNoPreservedMask());

thisMBB->addSuccessor(mainMBB);
thisMBB->addSuccessor(restoreMBB);

// mainMBB:
BuildMI(mainMBB, DL, TII->get(SystemZ::LHI), mainDstReg).addImm(0);
mainMBB->addSuccessor(sinkMBB);

// sinkMBB:
BuildMI(*sinkMBB, sinkMBB->begin(), DL, TII->get(SystemZ::PHI), DstReg)
.addReg(mainDstReg)
.addMBB(mainMBB)
.addReg(restoreDstReg)
.addMBB(restoreMBB);

// restoreMBB.
BuildMI(restoreMBB, DL, TII->get(SystemZ::LHI), restoreDstReg).addImm(1);
BuildMI(restoreMBB, DL, TII->get(SystemZ::J)).addMBB(sinkMBB);
restoreMBB->addSuccessor(sinkMBB);

MI.eraseFromParent();

return sinkMBB;
}

MachineBasicBlock *
SystemZTargetLowering::emitEHSjLjLongJmp(MachineInstr &MI,
MachineBasicBlock *MBB) const {

DebugLoc DL = MI.getDebugLoc();
const TargetInstrInfo *TII = Subtarget.getInstrInfo();

MachineFunction *MF = MBB->getParent();
MachineRegisterInfo &MRI = MF->getRegInfo();

MVT PVT = getPointerTy(MF->getDataLayout());
assert((PVT == MVT::i64 || PVT == MVT::i32) &&
"Invalid Pointer Size!");
Register BufReg = MI.getOperand(0).getReg();
const TargetRegisterClass *RC = MRI.getRegClass(BufReg);

Register Tmp = MRI.createVirtualRegister(RC);
Register BCReg = MRI.createVirtualRegister(RC);

MachineInstrBuilder MIB;

const int64_t FPOffset = 0;
const int64_t LabelOffset = 1 * PVT.getStoreSize();
const int64_t SPOffset = 3 * PVT.getStoreSize();
const int64_t LPOffset = 4 * PVT.getStoreSize();

MIB = BuildMI(*MBB, MI, DL, TII->get(SystemZ::LG), Tmp)
.addReg(BufReg)
.addImm(LabelOffset)
.addReg(0);

MIB = BuildMI(*MBB, MI, DL, TII->get(SystemZ::LG), SystemZ::R11D)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above about not hard-coding R11, R15 or the backchain offset here as well.

.addReg(BufReg)
.addImm(FPOffset)
.addReg(0);

// We are restoring R13 even though we never stored in setjmp from llvm,
// as gcc always stores R13 in builtin_setjmp. We could have mixed code
// gcc setjmp and llvm longjmp.
MIB = BuildMI(*MBB, MI, DL, TII->get(SystemZ::LG), SystemZ::R13D)
.addReg(BufReg)
.addImm(LPOffset)
.addReg(0);

bool BackChain = MF->getSubtarget<SystemZSubtarget>().hasBackChain();
if (BackChain) {
const int64_t BCOffset = 2 * PVT.getStoreSize();
MIB = BuildMI(*MBB, MI, DL, TII->get(SystemZ::LG), BCReg)
.addReg(BufReg)
.addImm(BCOffset)
.addReg(0);
}

MIB = BuildMI(*MBB, MI, DL, TII->get(SystemZ::LG), SystemZ::R15D)
.addReg(BufReg)
.addImm(SPOffset)
.addReg(0);

if (BackChain) {
BuildMI(*MBB, MI, DL, TII->get(SystemZ::STG))
.addReg(BCReg)
.addReg(SystemZ::R15D)
.addImm(0)
.addReg(0);
}

MIB = BuildMI(*MBB, MI, DL, TII->get(SystemZ::BR)).addReg(Tmp);

MI.eraseFromParent();
return MBB;
}

// Returns true if stack probing through inline assembly is requested.
bool SystemZTargetLowering::hasInlineStackProbe(const MachineFunction &MF) const {
// If the function specifically requests inline stack probes, emit them.
if (MF.getFunction().hasFnAttribute("probe-stack"))
Expand Down Expand Up @@ -6292,6 +6534,10 @@ SDValue SystemZTargetLowering::LowerOperation(SDValue Op,
return lowerGET_ROUNDING(Op, DAG);
case ISD::READCYCLECOUNTER:
return lowerREADCYCLECOUNTER(Op, DAG);
case ISD::EH_SJLJ_SETJMP:
case ISD::EH_SJLJ_LONGJMP:
return Op;

default:
llvm_unreachable("Unexpected node to lower");
}
Expand Down Expand Up @@ -9724,6 +9970,11 @@ MachineBasicBlock *SystemZTargetLowering::EmitInstrWithCustomInserter(

case SystemZ::PROBED_ALLOCA:
return emitProbedAlloca(MI, MBB);

case SystemZ::EH_SjLj_SetJmp:
return emitEHSjLjSetJmp(MI, MBB);
case SystemZ::EH_SjLj_LongJmp:
return emitEHSjLjLongJmp(MI, MBB);

case TargetOpcode::STACKMAP:
case TargetOpcode::PATCHPOINT:
Expand Down
7 changes: 7 additions & 0 deletions llvm/lib/Target/SystemZ/SystemZISelLowering.h
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,12 @@ class SystemZTargetLowering : public TargetLowering {
// LD, and having the full constant in memory enables reg/mem opcodes.
return VT != MVT::f64;
}
MachineBasicBlock *emitEHSjLjSetJmp(MachineInstr &MI,
MachineBasicBlock *MBB) const;

MachineBasicBlock *emitEHSjLjLongJmp(MachineInstr &MI,
MachineBasicBlock *MBB) const;

bool hasInlineStackProbe(const MachineFunction &MF) const override;
AtomicExpansionKind shouldCastAtomicLoadInIR(LoadInst *LI) const override;
AtomicExpansionKind shouldCastAtomicStoreInIR(StoreInst *SI) const override;
Expand Down Expand Up @@ -723,6 +729,7 @@ class SystemZTargetLowering : public TargetLowering {
SDValue lowerGET_ROUNDING(SDValue Op, SelectionDAG &DAG) const;
SDValue lowerREADCYCLECOUNTER(SDValue Op, SelectionDAG &DAG) const;


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be here.

bool canTreatAsByteVector(EVT VT) const;
SDValue combineExtract(const SDLoc &DL, EVT ElemVT, EVT VecVT, SDValue OrigOp,
unsigned Index, DAGCombinerInfo &DCI,
Expand Down
14 changes: 14 additions & 0 deletions llvm/lib/Target/SystemZ/SystemZInstrInfo.td
Original file line number Diff line number Diff line change
Expand Up @@ -1871,6 +1871,20 @@ let mayLoad = 1, mayStore = 1, Defs = [CC] in {
}
}

//--------------------------------------------------------------------------
// Setjmp/Longjmp.
//--------------------------------------------------------------------------
let isTerminator = 1, isBarrier = 1, isCodeGenOnly = 1, hasNoSchedulingInfo = 1, Size = 0 in {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lines here and below are a bit long and should be broken up - see other statements in the file as examples.

def EH_SjLj_Setup : Pseudo<(outs), (ins brtarget32:$dst), []>;
}

let hasSideEffects = 1, isBarrier = 1, usesCustomInserter = 1, hasNoSchedulingInfo = 1 in {
def EH_SjLj_SetJmp : Pseudo<(outs GR32:$dst), (ins ADDR64:$R2), [(set GR32:$dst, (z_eh_sjlj_setjmp ADDR64:$R2))]>;
}

let hasSideEffects = 1, isTerminator = 1, isBarrier = 1, usesCustomInserter = 1, hasNoSchedulingInfo = 1 in {
def EH_SjLj_LongJmp : Pseudo<(outs), (ins ADDR64:$R2), [(z_eh_sjlj_longjmp ADDR64:$R2)]>;
}
//===----------------------------------------------------------------------===//
// Message-security assist
//===----------------------------------------------------------------------===//
Expand Down
5 changes: 4 additions & 1 deletion llvm/lib/Target/SystemZ/SystemZLongBranch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,10 @@ static unsigned getInstSizeInBytes(const MachineInstr &MI,
MI.isImplicitDef() || MI.getOpcode() == TargetOpcode::MEMBARRIER ||
// These have a size that may be zero:
MI.isInlineAsm() || MI.getOpcode() == SystemZ::STACKMAP ||
MI.getOpcode() == SystemZ::PATCHPOINT) &&
MI.getOpcode() == SystemZ::PATCHPOINT ||
// EH_SjLj_Setup is a dummy terminator instruction of size 0,
// It is used to handle the clobber register for builtin setjmp.
MI.getOpcode() == SystemZ::EH_SjLj_Setup) &&
"Missing size value for instruction.");
return Size;
}
Expand Down
12 changes: 12 additions & 0 deletions llvm/lib/Target/SystemZ/SystemZOperators.td
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,12 @@ def SDT_ZTest : SDTypeProfile<1, 2,
[SDTCisVT<0, i32>,
SDTCisVT<2, i64>]>;

def SDT_ZSetJmp : SDTypeProfile<1, 1,
[SDTCisInt<0>,
SDTCisPtrTy<1>]>;
def SDT_ZLongJmp : SDTypeProfile<0, 1, [SDTCisPtrTy<0>]>;


//===----------------------------------------------------------------------===//
// Node definitions
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -314,6 +320,12 @@ def z_stckf : SDNode<"SystemZISD::STCKF", SDT_ZStoreInherent,

def z_tdc : SDNode<"SystemZISD::TDC", SDT_ZTest>;

def z_eh_sjlj_setjmp : SDNode<"ISD::EH_SJLJ_SETJMP", SDT_ZSetJmp,
[SDNPHasChain, SDNPSideEffect]>;
def z_eh_sjlj_longjmp : SDNode<"ISD::EH_SJLJ_LONGJMP", SDT_ZLongJmp,
[SDNPHasChain, SDNPSideEffect]>;


// Defined because the index is an i32 rather than a pointer.
def z_vector_insert : SDNode<"ISD::INSERT_VECTOR_ELT",
SDT_ZInsertVectorElt>;
Expand Down
Loading
Loading