[SSP] Remove llvm.stackprotectorcheck.

[timshen]

Summary

This is a cleanup patch for SSP support in LLVM. There is no functional change.
llvm.stackprotectorcheck is not needed, because SelectionDAG isn't
actually lowering it in SelectBasicBlock; rather, it adds check code in
FinishBasicBlock, ignoring the position where the intrinsic is inserted
(See FindSplitPointForStackProtector()).

[echristo]

I don't really understand why we need new intrinsics here. Can you explain the design a bit more? Can you also comment the intrinsics with what they do and how they're different from the existing ones?

Thanks!

-eric

[timshen]

tl;dr: because we have a million code paths. :)

The customization point in backends is the opcode LOAD_STACK_GUARD. It already exists for other reasons, but suitable for our purpose.

Previously we have two code paths to handle SSP, SelectionDAG path and IR path.

To customize stack guard loading in IR SSP, we need llvm.stackguardvalue. It lowers to LOAD_STACK_GUARD directly.

To customize stack guard loading in SelectionDAG, llvm.stackprotectorcheck has to take a value, so it doesn't have to be an address (e.g. could be a llvm.stackguardvalue call). To not break existing user code (it's documented) and tests, I created llvm.stackprotectorcheckvalue instead. llvm.stackprotectorcheck is not generated by LLVM anymore. See http://lists.llvm.org/pipermail/llvm-dev/2016-February/095760.html. We may want to delete llvm.stackprotectorcheck?

I haven't push all generic stack guard loading (typically loading the global variable __stack_chk_guard) to the backends. Once we push them all down, we can remove target-independent code path that deals with global variables. But we still need these two intrinsics for supporting both SelectionDAG and IR approach.

I added some comments for these two intrinsics.

[echristo]

Overall I think we're going to want a path that takes us through AutoUpgrade so that going forward we have a single path for all targets.

<insert about 30-45 minutes of discussion>

We're looking at a major design overhaul here, I think, so getting some notes down:

TLI->

lowerStackGuardStore
lowerStackGuardLoad
lowerStackCanaryLoad

We already have a guard node, do we need a separate SDag node for each of these?

Do we need this, effectively, duplicated 3x with SDag, FastIsel, Post-RA handling?

How do we keep the analysis information in the analysis pass and the rest of it in the IR? This would be the other side information like how to sort, strong, etc.

Need to continue to handle:

Coloring reuse
Sibcall optimizations
Avoid stack spill of canary values
Architecture neutral base case

and of course all of this with auto upgrade because I like my ponies to have unicorn horns.

[timshen]

I looked at LLVM manual and fortunately both llvm.stackprotector and
llvm.stackprotectorcheck is required to be passed in @__stack_chk_guard.
This limited flexibility allows us to directly change stackprotectorcheck's
signature and make an auto-upgrade for it.

In long term, I'd like to keep a diamond code path:

1. An IR pass that generates IR that calls llvm.stackprotector, llvm.stackprotectorcheck and llvm.experimental_stackguardvalue.
2. Either FastISel, or SelectionDAG handles the generated IR. Their shouldn't be an "IR approach" when FastISel isn't turned on (see the comment on TLI::supportsSelectionDAGSP).
3. Optional backend lowering is supported by overriding TLI::forceLoadStackGuardNode and TLI::getStackGuardAddr.

Notice that forceLoadStackGuardNode and getStackGuardAddr serves following
different purposes:

• Some backends want to lower the global variable manually, in such case forceLoadStackGuardNode should return true and getStackGuardAddr returns the global variable (search LOAD_STACK_GUARD in lib/Target).
• Other platforms, e.g. PowerPC 64 bit wants to lower the stack guard loading completely manually, getStackGuardAddr should returns nullptr.
• For the rest forceLoadStackGuardNode returns false and getStackGuardAddr returns the global variable.

[echristo]

Starting to look better, needs autoupgrade support and tests. I'm uncertain about putting one aspect of the set of intrinsics into experimental since the other two aren't already, but it's a simple change once we get the rest of it.

Can you add to the patch description a lot of the comments in the review? Also talk about the change of useLoadStackGuardNode to forceLoadStackGuardNode, etc.

Thanks!

-eric

[timshen]

As discussed, added documentation for StackProtector module. Also added a test case for auto-upgrade.

[echristo]

test/CodeGen/Generic/upgrade-stackprotectorcheck.ll

1 ↗

(On Diff #51614)

Should probably be in test/Assembler/auto_upgrade_intrinsics.ll or some such?

And you should check that the correct IR is generated at autoupgrade time.

[timshen]

Done. test/Bitcode seems to be a suitable place as well.

[echristo]

But there's already a large test that covers all sorts of intrinsics where I mentioned?

[timshen]

The last function created/touched in test/Assembler/auto_upgrade_intrinsics.ll is at 2013-10-07. This file has only three tests - seems not popular :).

What's the benefit of putting all test cases in one file?

[echristo]

For all of these? Because we're just parsing and disassembling, nothing complicated that'd be helped by multiple files and multiple process overhead.

[timshen]

Ah never mind, I guess test/Assembler/auto_upgrade_intrinsics.ll is a good place to hold target-independent intrinsic upgrades - there are actually not that many.

[echristo]

Cool. This is OK with me then.

Thanks for all of the hard work here.

[iteratee]

Some comments, mostly about comments and documentation.

include/llvm/IR/Intrinsics.td

329

I agree with eric that this shouldn't be experimental.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5360

I'm not sure I understand this comment.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

550–551

Is this still accurate? Are we keeping support for loading an address?

lib/CodeGen/StackProtector.cpp

405

I think this comment should read %1 = <load or intrinsic to read stack guard>

lib/Target/AArch64/AArch64ISelLowering.cpp

10104 ↗

(On Diff #51619)

Can you add a comment about why this needs to be overridden for this platform?

[timshen]

This comment was removed by timshen.

[timshen]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5360

Updated - simply forward the Value* to visitSPDescriptorParent, and inspect if it's a CallInst or a Load there.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

550–551

According to the documentation, we never support loading an arbitrary address. We only support loading @__stack_chk_guard. I would say in visitSPDescriptorParent, it doesn't have to check if the LoadInst is actually loading @__stack_chk_guard, because we are already doing more than that (loads @__guard_local on OpenBSD).

[timshen]

include/llvm/IR/Intrinsics.td

329

Why do you think removing experimental is safer? Isn't keeping it in any situation a safer move?

[timshen]

include/llvm/IR/Intrinsics.td

329

First safer -> better

[timshen]

Hi,

I realized that we don't really need llvm.stackguardcheck, so I try to come up with this patch as the first step of cleanup.

[timshen]

Also deleted documentation.

[iteratee]

Mostly looks good.

lib/CodeGen/StackProtector.cpp

420

Comment seems to belong to the instruction below. A comment about HasIRCheck would be nice too.

lib/Target/X86/X86ISelLowering.cpp

2141

The address space will go away in a later patch, correct?

[timshen]

Updated HasIRCheck comment.

lib/Target/X86/X86ISelLowering.cpp

2141

Yes, finally X86 should use LOAD_STACK_GUARD as well, but before that I'd like to introduce llvm.stackguard() intrinsic. After that point getStackGuardAddr isn't even needed.

[iteratee]

I have no more problems, and generally like your approach. It would be nice to get someone who has worked on this code before to sign off.

[timshen]

Hi Michael, since you wrote the SSP code (including stackprotectorcheck, according to git history), can you take a look at this cleanup?

Thanks!

[timshen]

Friendly ping. :)

[echristo]

Few inline comments and requests for more comments. Otherwise it's looking pretty nice.

Thanks!

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1935

Typical llvm style is: assert((cond) && "Some explanatory text")

(This happens in other places in your patch, I'll just include the first one)

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

1361 ↗

(On Diff #51844)

Comment.

lib/CodeGen/StackProtector.cpp

203

Standard style for this would be no braces for any of the conditionals or block.

lib/CodeGen/TargetLoweringBase.cpp

1748

Block comments on all of these if there aren't any in the .h file please? (I prefer more detailed comments on the implementation, but still).

test/Assembler/auto_upgrade_intrinsics.ll

57 ↗

(On Diff #51844)

Hmm?

[echristo]

Oh, and long as you add the inline comments, fix the style stuff, etc, I think we're good to go.

-eric

test/Assembler/auto_upgrade_intrinsics.ll

57 ↗

(On Diff #51844)

Nevermind, I see it now. That's frustrating, can you add a comment that the declare is for the objectsize upgrade and why?

[timshen]

Updated patch.

[timshen]

Thanks for the style check! It's a big patch and I clearly missed those.

Also rebased to include D18632's change.

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

1361 ↗

(On Diff #51844)

As suggested by gottesmm, moved this condition into StackProtector::shouldEmitSDCheck(), and documented.

[timshen]

Updated patch for fixing a typo.

[timshen]

Hi eugenis, I changed the function name getStackCookieLocation to getIRStackGuard, since there are two SSP code paths and such a name seems to cause less confusion. Do you think it's a fine name?

[eugenis]

In D17736#395796, @timshen wrote:

Hi eugenis, I changed the function name getStackCookieLocation to getIRStackGuard, since there are two SSP code paths and such a name seems to cause less confusion. Do you think it's a fine name?

Absolutely.

include/llvm/Target/TargetLowering.h

1008

This comment makes it sound like this function makes the decision to use SDAG vs IR stack protector.

[timshen]

Updated comment for getIRStackGuard.

include/llvm/Target/TargetLowering.h

1008

Moved the comment to StackProtector::CreatePrologue. At that point I was not aware that other code will use this function.

[eugenis]

looks good

include/llvm/IR/Intrinsics.td

 def int_readcyclecounter : Intrinsic<[llvm_i64_ty]>;
 
 // The assume intrinsic is marked as arbitrarily writing so that proper
 // control dependencies will be maintained.
 def int_assume        : Intrinsic<[], [llvm_i1_ty], []>;
 
 // Stack Protector Intrinsic - The stackprotector intrinsic writes the stack
 // guard to the correct place on the stack frame.
+//
+// stackprotectorcheck takes an address to stack guard, load the value and
+// compare against the canary value on the stack. It's kept only because it's
+// documented and publicly usable. It's never generated by LLVM SSP.
+//
+// LLVM stack protector generates a more general form stackprotectorcheckvalue,
+// which carries a IR that evaluates to a stack guard value.
+//
+// stackprotectorcheck* is only used when -enable-selectiondag-sp is on and
+// target supports SSP in SelectionDAG. Otherwise SSP pass will generate IR
+// level code, including stackguardvalue calls. Backends may lower
+// stackguardvalue in the same way they do in stackprotectorcheckvalue (by
+// implementing LOAD_STACK_GUARD pseudo instruction).
 def int_stackprotector : Intrinsic<[], [llvm_ptr_ty, llvm_ptrptr_ty], []>;
+// TODO: Remove this?
 def int_stackprotectorcheck : Intrinsic<[], [llvm_ptrptr_ty],
                                         [IntrReadWriteArgMem]>;
+def int_stackprotectorcheckvalue : Intrinsic<[], [llvm_ptr_ty],
+                                             [IntrReadWriteArgMem]>;
+def int_stackguardvalue : Intrinsic<[llvm_ptr_ty], [], [IntrReadWriteArgMem]>;
 
 // A counter increment for instrumentation based profiling.
 def int_instrprof_increment : Intrinsic<[],
                                         [llvm_ptr_ty, llvm_i64_ty,
                                          llvm_i32_ty, llvm_i32_ty],
                                         []>;
 
 // A call to profile runtime for value profiling of target expressions

include/llvm/Target/TargetLowering.h

public:
   }
 
   /// Return whether the DAG builder should automatically insert fences and
   /// reduce ordering for atomics.
   bool getInsertFencesForAtomic() const {
     return InsertFencesForAtomic;
   }
 
-  /// Return true if the target stores stack protector cookies at a fixed offset
-  /// in some non-standard address space, and populates the address space and
-  /// offset as appropriate.
-  virtual bool getStackCookieLocation(unsigned &/*AddressSpace*/,
-                                      unsigned &/*Offset*/) const {
-    return false;
+  /// Return whether the target supports selection dag based stack protector.
+  virtual bool supportsSelectionDAGSP() const {
+    return !getTargetMachine().getTargetTriple().isOSOpenBSD();
   }
 
+  /// Return the address of stack guard, or return nullptr if the target wants
+  /// to lower LOAD_STACK_GUARD in a target specific manner.
+  virtual Value *getStackGuardAddr(Module &M) const;
+
   /// If the target has a standard location for the unsafe stack pointer,
   /// returns the address of that location. Otherwise, returns nullptr.
   virtual Value *getSafeStackPointerLocation(IRBuilder<> &IRB) const;
 
   /// Returns true if a cast between SrcAS and DestAS is a noop.
   virtual bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const {
     return false;
   }
public:
   /// the 'hasPostISelHook' flag. These instructions must be adjusted after
   /// instruction selection by target hooks.  e.g. To fill in optional defs for
   /// ARM 's' setting instructions.
   virtual void
   AdjustInstrPostInstrSelection(MachineInstr *MI, SDNode *Node) const;
 
   /// If this function returns true, SelectionDAGBuilder emits a
   /// LOAD_STACK_GUARD node when it is lowering Intrinsic::stackprotector.
+  /// TODO: This function seems to be overlapped with getStackGuardAddr().
+  ///       Maybe we can remove this one?
   virtual bool useLoadStackGuardNode() const {
     return false;
   }
 
   /// Lower TLS global address SDNode for target independent emulated TLS model.
   virtual SDValue LowerToTLSEmulatedModel(const GlobalAddressSDNode *GA,
                                           SelectionDAG &DAG) const;
 };

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

public:
     /// Returns true if all fields of the stack protector descriptor are
     /// initialized implying that we should/are ready to emit a stack protector.
     bool shouldEmitStackProtector() const {
       return ParentMBB && SuccessMBB && FailureMBB && Guard;
     }
 
     /// Initialize the stack protector descriptor structure for a new basic
     /// block.
-    void initialize(const BasicBlock *BB,
-                    MachineBasicBlock *MBB,
-                    const CallInst &StackProtCheckCall) {
+    void initialize(const BasicBlock *BB, MachineBasicBlock *MBB,
+                    const Value *G) {
       // Make sure we are not initialized yet.
       assert(!shouldEmitStackProtector() && "Stack Protector Descriptor is "
              "already initialized!");
       ParentMBB = MBB;
       SuccessMBB = AddSuccessorMBB(BB, MBB, /* IsLikely */ true);
       FailureMBB = AddSuccessorMBB(BB, MBB, /* IsLikely */ false, FailureMBB);
       if (!Guard)
-        Guard = StackProtCheckCall.getArgOperand(0);
+        Guard = G;
     }
 
     /// Reset state that changes when we handle different basic blocks.
     ///
     /// This currently includes:
     ///
     /// 1. The specific basic block we are generating a
     /// stack protector for (ParentMBB).
private:
     /// A basic block visited on stack protector check success that contains the
     /// terminators of ParentMBB.
     MachineBasicBlock *SuccessMBB;
 
     /// This basic block visited on stack protector check failure that will
     /// contain a call to __stack_chk_fail().
     MachineBasicBlock *FailureMBB;
 
-    /// The guard variable which we will compare against the stored value in the
-    /// stack protector stack slot.
+    /// It either stores an address to the stack guard, or stores a
+    /// llvm.stackguardvalue CallInst.
     const Value *Guard;
 
     /// The virtual register holding the stack guard value.
     unsigned GuardReg;
 
     /// Add a successor machine basic block to ParentMBB. If the successor mbb
     /// has not been created yet (i.e. if SuccMBB = 0), then the machine basic
     /// block will be created. Assign a large weight if IsLikely is true.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitSPDescriptorParent(StackProtectorDescriptor &SPD,
   // First create the loads to the guard/stack slot for the comparison.
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   EVT PtrTy = TLI.getPointerTy(DAG.getDataLayout());
 
   MachineFrameInfo *MFI = ParentBB->getParent()->getFrameInfo();
   int FI = MFI->getStackProtectorIndex();
 
   const Value *IRGuard = SPD.getGuard();
-  SDValue GuardPtr = getValue(IRGuard);
-  SDValue StackSlotPtr = DAG.getFrameIndex(FI, PtrTy);
-
   unsigned Align = DL->getPrefTypeAlignment(IRGuard->getType());
-
-  SDValue Guard;
   SDLoc dl = getCurSDLoc();
 
+  SDValue Guard;
   // If GuardReg is set and useLoadStackGuardNode returns true, retrieve the
   // guard value from the virtual register holding the value. Otherwise, emit a
   // volatile load to retrieve the stack guard value.
   unsigned GuardReg = SPD.getGuardReg();
-
-  if (GuardReg && TLI.useLoadStackGuardNode())
+  if (GuardReg && TLI.useLoadStackGuardNode()) {
     Guard = DAG.getCopyFromReg(DAG.getEntryNode(), dl, GuardReg,
                                PtrTy);
-  else
+  } else if (dyn_cast<CallInst>(IRGuard)) {
+    Guard = getValue(IRGuard);
+  } else {
+    SDValue GuardPtr = getValue(IRGuard);
     Guard = DAG.getLoad(PtrTy, dl, DAG.getEntryNode(),
                         GuardPtr, MachinePointerInfo(IRGuard, 0),
                         true, false, false, Align);
+  }
 
+  SDValue StackSlotPtr = DAG.getFrameIndex(FI, PtrTy);
   SDValue StackSlot = DAG.getLoad(
       PtrTy, dl, DAG.getEntryNode(), StackSlotPtr,
       MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI), true,
       false, false, Align);
 
   // Perform the comparison via a subtract/getsetcc.
   EVT VT = Guard.getValueType();
   SDValue Sub = DAG.getNode(ISD::SUB, dl, VT, Guard, StackSlot);
if (PtrTy != ResTy)
       report_fatal_error("Wrong result type for @llvm.get.dynamic.area.offset"
                          " intrinsic!");
     Res = DAG.getNode(ISD::GET_DYNAMIC_AREA_OFFSET, sdl, DAG.getVTList(ResTy),
                       Op);
     DAG.setRoot(Op);
     setValue(&I, Res);
     return nullptr;
   }
+  case Intrinsic::stackguardvalue: {
+    EVT PtrTy = TLI.getPointerTy(DAG.getDataLayout());
+    SDValue Chain = getRoot();
+    MachineSDNode *Node =
+        DAG.getMachineNode(TargetOpcode::LOAD_STACK_GUARD, sdl, PtrTy, Chain);
+    DAG.setRoot(Chain);
+    setValue(&I, SDValue(Node, 0));
+    return nullptr;
+  }
   case Intrinsic::stackprotector: {
     // Emit code into the DAG to store the stack guard onto the stack.
     MachineFunction &MF = DAG.getMachineFunction();
     MachineFrameInfo *MFI = MF.getFrameInfo();
     EVT PtrTy = TLI.getPointerTy(DAG.getDataLayout());
     SDValue Src, Chain = getRoot();
-    const Value *Ptr = cast<LoadInst>(I.getArgOperand(0))->getPointerOperand();
-    const GlobalVariable *GV = dyn_cast<GlobalVariable>(Ptr);
+    const GlobalVariable *GV = nullptr;
+    if (auto Ptr = dyn_cast<LoadInst>(I.getArgOperand(0))) {
+      GV = dyn_cast<GlobalVariable>(Ptr->getPointerOperand());
 
-    // See if Ptr is a bitcast. If it is, look through it and see if we can get
-    // global variable __stack_chk_guard.
+      // See if Ptr is a bitcast. If it is, look through it and see if we can
+      // get global variable __stack_chk_guard.
       if (!GV)
-      if (const Operator *BC = dyn_cast<Operator>(Ptr))
+        if (const Operator *BC = dyn_cast<Operator>(Ptr->getPointerOperand()))
           if (BC->getOpcode() == Instruction::BitCast)
             GV = dyn_cast<GlobalVariable>(BC->getOperand(0));
+    }
 
     if (GV && TLI.useLoadStackGuardNode()) {
       // Emit a LOAD_STACK_GUARD node.
       MachineSDNode *Node = DAG.getMachineNode(TargetOpcode::LOAD_STACK_GUARD,
                                                sdl, PtrTy, Chain);
       MachinePointerInfo MPInfo(GV);
       MachineInstr::mmo_iterator MemRefs = MF.allocateMemRefsArray(1);
       unsigned Flags = MachineMemOperand::MOLoad |
                        MachineMemOperand::MOInvariant;
       *MemRefs = MF.getMachineMemOperand(MPInfo, Flags,
                                          PtrTy.getSizeInBits() / 8,
                                          DAG.getEVTAlignment(PtrTy));
       Node->setMemRefs(MemRefs, MemRefs + 1);
 
-      // Copy the guard value to a virtual register so that it can be
-      // retrieved in the epilogue.
       Src = SDValue(Node, 0);
+    } else {
+      Src = getValue(I.getArgOperand(0)); // The guard's value.
+    }
+    // We get a LOAD_STCK_GUARD either from useLoadStackGuardNode or from
+    // lowering llvm.stackguardvalue. Copy the guard value to a virtual
+    // register so that it can be retrieved in the epilogue.
+    if ((GV && TLI.useLoadStackGuardNode()) ||
+        dyn_cast<CallInst>(I.getArgOperand(0))) {
       const TargetRegisterClass *RC =
           TLI.getRegClassFor(Src.getSimpleValueType());
       unsigned Reg = MF.getRegInfo().createVirtualRegister(RC);
 
       SPDescriptor.setGuardReg(Reg);
       Chain = DAG.getCopyToReg(Chain, sdl, Reg, Src);
-    } else {
-      Src = getValue(I.getArgOperand(0));   // The guard's value.
     }
 
     AllocaInst *Slot = cast<AllocaInst>(I.getArgOperand(1));
 
     int FI = FuncInfo.StaticAllocaMap[Slot];
     MFI->setStackProtectorIndex(FI);
 
     SDValue FIN = DAG.getFrameIndex(FI, PtrTy);
SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I, unsigned Intrinsic) {
   }
   case Intrinsic::invariant_start:
     // Discard region information.
     setValue(&I, DAG.getUNDEF(TLI.getPointerTy(DAG.getDataLayout())));
     return nullptr;
   case Intrinsic::invariant_end:
     // Discard region information.
     return nullptr;
-  case Intrinsic::stackprotectorcheck: {
+  case Intrinsic::stackprotectorcheck:
+  case Intrinsic::stackprotectorcheckvalue: {
     // Do not actually emit anything for this basic block. Instead we initialize
     // the stack protector descriptor and export the guard variable so we can
     // access it in FinishBasicBlock.
     const BasicBlock *BB = I.getParent();
-    SPDescriptor.initialize(BB, FuncInfo.MBBMap[BB], I);
+    // llvm.stackprotectorcheck is handled only for backward compatibility.
+    // It always carries an address.
+    // llvm.stackprotectorcheckvalue may carry a load of the address,
+    // or a llvm.stackguardvalue call.
+    // SPDescriptor expects either an address, or an intrinsic call, so we need
+    // to strip the LoadInst if possible.
+    if (auto Load = dyn_cast<LoadInst>(I.getArgOperand(0)))
+      SPDescriptor.initialize(BB, FuncInfo.MBBMap[BB],
+                              Load->getPointerOperand());
+    else
+      SPDescriptor.initialize(BB, FuncInfo.MBBMap[BB], I.getArgOperand(0));
     ExportFromCurrentBlock(SPDescriptor.getGuard());
 
     // Flush our exports since we are going to process a terminator.
     (void)getControlRoot();
     return nullptr;
   }
   case Intrinsic::clear_cache:
     return TLI.getClearCacheBuiltinName();

lib/CodeGen/StackProtector.cpp

 /// strong heuristic will add a guard variables to functions that call alloca
 /// regardless of size, functions with any buffer regardless of type and size,
 /// functions with aggregates that contain any buffer regardless of type and
 /// size, and functions that contain stack-based variables that have had their
 /// address taken.
 bool StackProtector::RequiresStackProtector() {
   bool Strong = false;
   bool NeedsProtector = false;
   if (F->hasFnAttribute(Attribute::StackProtectReq)) {
     NeedsProtector = true;
     Strong = true; // Use the same heuristic as strong to determine SSPLayout
   } else if (F->hasFnAttribute(Attribute::StackProtectStrong))
     Strong = true;
   else if (!F->hasFnAttribute(Attribute::StackProtect))
     return false;
 
   for (const BasicBlock &BB : *F) {
for (BasicBlock::reverse_iterator I = std::next(BB->rbegin()), E = BB->rend();
 
     // Increment max search.
     SearchCounter++;
   }
 
   return nullptr;
 }
 
-/// Insert code into the entry block that stores the __stack_chk_guard
+/// Return the instruction to load a stack guard.
+static Value *CreateLoadStackGuard(IRBuilder<> &B, Module &M,
+                                   const TargetLoweringBase *TLI) {
+  if (auto StackGuardAddr = TLI->getStackGuardAddr(M))
+    return B.CreateLoad(StackGuardAddr, "StackGuard");
+  return B.CreateCall(
+      Intrinsic::getDeclaration(&M, Intrinsic::stackguardvalue));
+}
+
+/// Insert code into the entry block that stores the stack guard
 /// variable onto the stack:
 ///
 ///   entry:
 ///     StackGuardSlot = alloca i8*
-///     StackGuard = load __stack_chk_guard
-///     call void @llvm.stackprotect.create(StackGuard, StackGuardSlot)
-///
-/// Returns true if the platform/triple supports the stackprotectorcreate pseudo
-/// node.
-static bool CreatePrologue(Function *F, Module *M, ReturnInst *RI,
+///     StackGuard = <value of the stack guard>
+///     call void @llvm.stackprotector(StackGuard, StackGuardSlot)
+static void CreatePrologue(Function *F, Module *M, ReturnInst *RI,
                            const TargetLoweringBase *TLI, const Triple &TT,
-                           AllocaInst *&AI, Value *&StackGuardVar) {
-  bool SupportsSelectionDAGSP = false;
+                           AllocaInst *&AI, Value *&StackGuardVal) {
   PointerType *PtrTy = Type::getInt8PtrTy(RI->getContext());
-  unsigned AddressSpace, Offset;
-  if (TLI->getStackCookieLocation(AddressSpace, Offset)) {
-    Constant *OffsetVal =
-        ConstantInt::get(Type::getInt32Ty(RI->getContext()), Offset);
-
-    StackGuardVar =
-        ConstantExpr::getIntToPtr(OffsetVal, PointerType::get(PtrTy,
-                                                              AddressSpace));
-  } else if (TT.isOSOpenBSD()) {
-    StackGuardVar = M->getOrInsertGlobal("__guard_local", PtrTy);
-    cast<GlobalValue>(StackGuardVar)
-        ->setVisibility(GlobalValue::HiddenVisibility);
-  } else {
-    SupportsSelectionDAGSP = true;
-    StackGuardVar = M->getOrInsertGlobal("__stack_chk_guard", PtrTy);
-  }
 
   IRBuilder<> B(&F->getEntryBlock().front());
   AI = B.CreateAlloca(PtrTy, nullptr, "StackGuardSlot");
-  LoadInst *LI = B.CreateLoad(StackGuardVar, "StackGuard");
+  StackGuardVal = CreateLoadStackGuard(B, *M, TLI);
   B.CreateCall(Intrinsic::getDeclaration(M, Intrinsic::stackprotector),
-               {LI, AI});
-
-  return SupportsSelectionDAGSP;
+               {StackGuardVal, AI});
 }
 
 /// InsertStackProtectors - Insert code into the prologue and epilogue of the
 /// function.
 ///
 ///  - The prologue code loads and stores the stack guard onto the stack.
 ///  - The epilogue checks the value stored in the prologue against the original
 ///    value. It calls __stack_chk_fail if they differ.
 bool StackProtector::InsertStackProtectors() {
   bool HasPrologue = false;
-  bool SupportsSelectionDAGSP =
-      EnableSelectionDAGSP && !TM->Options.EnableFastISel;
+  bool SupportsSelectionDAGSP = EnableSelectionDAGSP &&
+                                !TM->Options.EnableFastISel &&
+                                TLI->supportsSelectionDAGSP();
   AllocaInst *AI = nullptr;       // Place on stack that stores the stack guard.
-  Value *StackGuardVar = nullptr; // The stack guard variable.
+  Value *StackGuardVal = nullptr; // The stack guard value.
 
   for (Function::iterator I = F->begin(), E = F->end(); I != E;) {
     BasicBlock *BB = &*I++;
     ReturnInst *RI = dyn_cast<ReturnInst>(BB->getTerminator());
     if (!RI)
       continue;
 
     if (!HasPrologue) {
       HasPrologue = true;
-      SupportsSelectionDAGSP &=
-          CreatePrologue(F, M, RI, TLI, Trip, AI, StackGuardVar);
+      CreatePrologue(F, M, RI, TLI, Trip, AI, StackGuardVal);
     }
 
     if (SupportsSelectionDAGSP) {
       // Since we have a potential tail call, insert the special stack check
       // intrinsic.
       Instruction *InsertionPt = nullptr;
       if (CallInst *CI = FindPotentialTailCall(BB, RI, TLI)) {
         InsertionPt = CI;
       } else {
         InsertionPt = RI;
         // At this point we know that BB has a return statement so it *DOES*
         // have a terminator.
         assert(InsertionPt != nullptr &&
                "BB must have a terminator instruction at this point.");
       }
 
       Function *Intrinsic =
-          Intrinsic::getDeclaration(M, Intrinsic::stackprotectorcheck);
-      CallInst::Create(Intrinsic, StackGuardVar, "", InsertionPt);
+          Intrinsic::getDeclaration(M, Intrinsic::stackprotectorcheckvalue);
+      CallInst::Create(Intrinsic, StackGuardVal, "", InsertionPt);
     } else {
       // If we do not support SelectionDAG based tail calls, generate IR level
       // tail calls.
       //
       // For each block with a return instruction, convert this:
       //
       //   return:
       //     ...
       //     ret ...
       //
       // into this:
       //
       //   return:
       //     ...
-      //     %1 = load __stack_chk_guard
+      //     %1 = <value of the stack guard>
       //     %2 = load StackGuardSlot
       //     %3 = cmp i1 %1, %2
       //     br i1 %3, label %SP_return, label %CallStackCheckFailBlk
       //
       //   SP_return:
       //     ret ...
       //
       //   CallStackCheckFailBlk:
       //     call void @__stack_chk_fail()
       //     unreachable
 
       // Create the FailBB. We duplicate the BB every time since the MI tail
       // merge pass will merge together all of the various BB into one including
       // fail BB generated by the stack protector pseudo instruction.
       BasicBlock *FailBB = CreateFailBB();
 
       // Split the basic block before the return instruction.
       BasicBlock *NewBB = BB->splitBasicBlock(RI->getIterator(), "SP_return");
 
       // Update the dominator tree if we need to.
       if (DT && DT->isReachableFromEntry(BB)) {
         DT->addNewBlock(NewBB, BB);
         DT->addNewBlock(FailBB, BB);
       }
 
       // Remove default branch instruction to the new BB.
       BB->getTerminator()->eraseFromParent();
 
       // Move the newly created basic block to the point right after the old
       // basic block so that it's in the "fall through" position.
       NewBB->moveAfter(BB);
 
       // Generate the stack protector instructions in the old basic block.
       IRBuilder<> B(BB);
-      LoadInst *LI1 = B.CreateLoad(StackGuardVar);
+      Value *LI1 = CreateLoadStackGuard(B, *M, TLI);
       LoadInst *LI2 = B.CreateLoad(AI);
       Value *Cmp = B.CreateICmpEQ(LI1, LI2);
       auto SuccessProb =
           BranchProbabilityInfo::getBranchProbStackProtector(true);
       auto FailureProb =
           BranchProbabilityInfo::getBranchProbStackProtector(false);
       MDNode *Weights = MDBuilder(F->getContext())
                             .createBranchWeights(SuccessProb.getNumerator(),

lib/CodeGen/TargetLoweringBase.cpp

case 2:
     // Allow 2*r as r+r.
     break;
   default: // Don't allow n * r
     return false;
   }
 
   return true;
 }
+
+Value *TargetLoweringBase::getStackGuardAddr(Module &M) const {
+  PointerType *PtrTy = Type::getInt8PtrTy(M.getContext());
+  if (getTargetMachine().getTargetTriple().isOSOpenBSD()) {
+    auto StackGuardAddr = M.getOrInsertGlobal("__guard_local", PtrTy);
+    cast<GlobalValue>(StackGuardAddr)
+        ->setVisibility(GlobalValue::HiddenVisibility);
+    return StackGuardAddr;
+  }
+  return M.getOrInsertGlobal("__stack_chk_guard", PtrTy);
+}

lib/Target/X86/X86ISelLowering.h

public:
 
     virtual bool needsFixedCatchObjects() const override;
 
     /// This method returns a target specific FastISel object,
     /// or null if the target does not support "fast" ISel.
     FastISel *createFastISel(FunctionLoweringInfo &funcInfo,
                              const TargetLibraryInfo *libInfo) const override;
 
-    /// Return true if the target stores stack protector cookies at a fixed
-    /// offset in some non-standard address space, and populates the address
-    /// space and offset as appropriate.
-    bool getStackCookieLocation(unsigned &AddressSpace,
-                                unsigned &Offset) const override;
+    bool supportsSelectionDAGSP() const override;
+
+    Value *getStackGuardAddr(Module &M) const override;
 
     /// Return true if the target stores SafeStack pointer at a fixed offset in
     /// some non-standard address space, and populates the address space and
     /// offset as appropriate.
     Value *getSafeStackPointerLocation(IRBuilder<> &IRB) const override;
 
     SDValue BuildFILD(SDValue Op, EVT SrcVT, SDValue Chain, SDValue StackSlot,
                       SelectionDAG &DAG) const;

lib/Target/X86/X86ISelLowering.cpp

 }
 
 unsigned X86TargetLowering::getAddressSpace() const {
   if (Subtarget.is64Bit())
     return (getTargetMachine().getCodeModel() == CodeModel::Kernel) ? 256 : 257;
   return 256;
 }
 
-bool X86TargetLowering::getStackCookieLocation(unsigned &AddressSpace,
-                                               unsigned &Offset) const {
-  if (!Subtarget.isTargetLinux())
+bool X86TargetLowering::supportsSelectionDAGSP() const {
+  if (Subtarget.isTargetLinux())
     return false;
+  return TargetLowering::supportsSelectionDAGSP();
+}
 
+Value *X86TargetLowering::getStackGuardAddr(Module &M) const {
+  /// The target stores stack protector cookies at a fixed
+  /// offset in some non-standard address space, and populates the address
+  /// space and offset as appropriate.
+  if (Subtarget.isTargetLinux()) {
+    unsigned AddressSpace = getAddressSpace();
     // %fs:0x28, unless we're using a Kernel code model, in which case it's %gs:
     // %gs:0x14 on i386
-  Offset = (Subtarget.is64Bit()) ? 0x28 : 0x14;
-  AddressSpace = getAddressSpace();
-  return true;
+    unsigned Offset = (Subtarget.is64Bit()) ? 0x28 : 0x14;
+    Constant *OffsetVal =
+        ConstantInt::get(Type::getInt32Ty(M.getContext()), Offset);
+
+    PointerType *PtrTy = Type::getInt8PtrTy(M.getContext());
+    return ConstantExpr::getIntToPtr(OffsetVal,
+                                     PointerType::get(PtrTy, AddressSpace));
+  }
+  return TargetLowering::getStackGuardAddr(M);
 }
 
 Value *X86TargetLowering::getSafeStackPointerLocation(IRBuilder<> &IRB) const {
   if (!Subtarget.isTargetAndroid())
     return TargetLowering::getSafeStackPointerLocation(IRB);
 
   // Android provides a fixed TLS slot for the SafeStack pointer. See the
   // definition of TLS_SLOT_SAFESTACK in