zbrid added a comment.
Looks great! Thanks for writing this! I had a bunch of nits (sorry!) and a few
questions, otherwise LGTM. Please wait for sign off from at least one other
person before submitting.
================
Comment at: llvm/lib/Target/X86/X86FastISel.cpp:3210
// Functions using retpoline for indirect calls need to use SDISel.
+ if (Subtarget->useRetpolineIndirectCalls() ||
----------------
nit: Update comment?
================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:966
+ if (Is64Bit && IsLargeCodeModel && (STI.useRetpolineIndirectCalls() ||
+ STI.useLVIControlFlowIntegrity()))
report_fatal_error("Emitting stack probe calls on 64-bit with the large "
----------------
Would it make sense to add a separate check for LVI-CFI and have a
corresponding error message referencing specifically LVI-CFI rather than
retpolines?
---
I wrote this in several spots, sorry about the repetition.
================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:2707
// FIXME: Add retpoline support and remove the error here..
- if (STI.useRetpolineIndirectCalls())
+ if (STI.useRetpolineIndirectCalls() || STI.useLVIControlFlowIntegrity())
report_fatal_error("Emitting morestack calls on 64-bit with the large "
----------------
Would it make sense to add a separate check for LVI-CFI and have a
corresponding error message referencing specifically LVI-CFI rather than
retpolines?
================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30557
bool X86TargetLowering::areJTsAllowed(const Function *Fn) const {
// If the subtarget is using retpolines, we need to not generate jump tables.
+ if (Subtarget.useRetpolineIndirectBranches() ||
----------------
nit: Update comment to mention LVI-CFI
================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31844
MachineBasicBlock *
X86TargetLowering::EmitLoweredRetpoline(MachineInstr &MI,
MachineBasicBlock *BB) const {
----------------
Can you change this function name? If I'm understanding this code correctly,
this no longer only applies to retpoline, but also to LVI thunks, so the naming
is misleading.
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:79
+
+bool X86LoadValueInjectionIndirectThunksPass::doInitialization(Module &M) {
+ InsertedThunks = false;
----------------
I want to make sure I understand this correctly: You use this function to
initialize InsertedThunks so that, for each module, InsertedThunks is shared
across all the functions. This enables the Module to ensure the thunk is only
inserted once. Is that right?
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:87
+ STI = &MF.getSubtarget<X86Subtarget>();
+ if (!(STI->hasSSE2() || STI->is64Bit())) {
+ // FIXME: support 32-bit
----------------
Why is 32-bit okay if it has SSE2 features? (Asking to understand since my
processor knowledge is a bit weak. Thanks!)
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:92
+
+ // Don't skip functions with the "optnone" attr but participate in
opt-bisect.
+ const Function &F = MF.getFunction();
----------------
Why did you decide to make this not skip functions with optnone?
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:98
+
+ LLVM_DEBUG(dbgs() << "***** " << getPassName() << " : " << MF.getName()
+ << " *****\n");
----------------
I think this debugging message should be at the top of the function.
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:129
+
+ assert(MF.getName() == "__x86_indirect_thunk_r11" &&
+ "Should only have an r11 thunk on 64-bit targets");
----------------
Should this use R11ThunkName instead of this string literal?
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:149
+ // inline.
+ AttrBuilder B;
+ B.addAttribute(llvm::Attribute::NoUnwind);
----------------
I see this list is from the retpoline pass. I don't know what each of these
things do, but just wondering if you double checked these are the same
attributes we want for this thunk?
================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1227
+ if (Subtarget->useRetpolineIndirectCalls() ||
+ Subtarget->useLVIControlFlowIntegrity())
report_fatal_error("Lowering register statepoints with retpoline not "
----------------
Would it make sense to add a separate check for LVI-CFI and have a
corresponding error message referencing specifically LVI-CFI rather than
retpolines?
================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1407
+ if (Subtarget->useRetpolineIndirectCalls() ||
+ Subtarget->useLVIControlFlowIntegrity())
report_fatal_error(
----------------
Would it make sense to add a separate check for LVI-CFI and have a
corresponding error message referencing specifically LVI-CFI rather than
retpolines?
================
Comment at: llvm/test/CodeGen/X86/O0-pipeline.ll:76
; CHECK-NEXT: X86 Retpoline Thunks
+; CHECK-NEXT: X86 Load Value Injection (LVI) Indirect Thunks Pass
; CHECK-NEXT: Check CFA info and insert CFI instructions if needed
----------------
nit: remove pass to follow what most of the other passes do here (requires
change in pass code and here)
================
Comment at: llvm/test/CodeGen/X86/O3-pipeline.ll:185
; CHECK-NEXT: X86 Retpoline Thunks
+; CHECK-NEXT: X86 Load Value Injection (LVI) Indirect Thunks Pass
; CHECK-NEXT: Check CFA info and insert CFI instructions if needed
----------------
Same comment as in O0-pipeline.ll test
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75934/new/
https://reviews.llvm.org/D75934
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
