[PATCH] D46181: [X86][CET] Shadow stack fix for setjmp/longjmp

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27560
+  unsigned SSPCopyReg = MRI.createVirtualRegister(PtrRC);
+  unsigned rdssp = (PVT == MVT::i64) ? X86::RDSSPQ : X86::RDSSPD;
+  BuildMI(*MBB, MI, DL, TII->get(rdssp), SSPCopyReg).addReg(ZReg);
----------------
Variable names should be capitalized.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27797
+  unsigned ZReg = MRI.createVirtualRegister(PtrRC);
+  unsigned MovRIOpc = (PVT == MVT::i64) ? X86::MOV64ri : X86::MOV32ri;
+  BuildMI(checkSspMBB, DL, TII->get(MovRIOpc), ZReg).addImm(0);
----------------
Can we use MOV64ri32 instead of MOV64ri? That would only use 32-bits for the immediate instead of 64.

Or better yet, can you use XOR?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27807
+  // to the sink.
+  unsigned CmpRIOpc = (PVT == MVT::i64) ? X86::CMP64ri32 : X86::CMP32ri;
+  BuildMI(checkSspMBB, DL, TII->get(CmpRIOpc)).addReg(SSPCopyReg).addImm(0);
----------------
Can the be "TEST reg, reg" instead of CMP? Should be shorter encoding that putting 0 in the immediate. Even if not CMP64ri8/CMP32ri8 should be shorter than the ri32/ri versions.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27863
+  // Do a single shift left.
+  unsigned ShlRIOpc = (PVT == MVT::i64) ? X86::SHL64ri : X86::SHL32ri;
+  unsigned SspAfterShlReg = MRI.createVirtualRegister(PtrRC);
----------------
Single shifts should use SHL64r1/SHL32r1


Repository:
  rL LLVM

https://reviews.llvm.org/D46181