kristof.beyls accepted this revision.
kristof.beyls added a comment.
I'm not an expert on many of the areas touched by this patch, but it looks fine
from me from a high-level point-of-view, modulo a few nits I have on a few
comments.
================
Comment at: clang/lib/Driver/ToolChains/Arch/X86.cpp:151
+ // flags). This is a bit hacky but keeps existing usages working. We should
+ // consider deprecated this and instead warning if the user requests external
+ // retpoline thunks and *doesn't* request some form of retpolines.
----------------
s/deprecated/deprecating/
s/warning/warn/
================
Comment at: llvm/include/llvm/IR/Attributes.td:181-185
+/// Note that this uses the default compatibility (always compatible during
+/// inlining) and the default merge strategy of retaining the caller's
+/// attribute. This specifically matches the intent for this attribute which is
+/// that the context dominates, and inlined code will become hardened or lose
+/// its hardening based on the caller's attribute.
----------------
After updating the LangRef.rst text, I think this comment also needs to be
updated. As is, it still documents the old inlining behaviour?
I'm also not sure in how far the comment makes most sense here. This is already
documented in LangRef.rst, and AFAIK, the inlining compatibility mode is not
something that is defined here?
================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:78-82
+static cl::opt<bool> EnableSpeculativeLoadHardening(
+ "x86-speculative-load-hardening",
+ cl::desc("Force enable speculative load hardening"), cl::init(false),
+ cl::Hidden);
+
----------------
I'm not sure, but do you really still need an option to force enable SLH when
you have function attributes now to enable it?
When you generate LLVM-IR using clang, you now have a clang option to enable
that function attribute on all functions, so do you still have scenarios where
you need an LLVM backend option to override the function attribute?
================
Comment at: llvm/lib/Target/X86/X86TargetMachine.cpp:474
- if (EnableSpeculativeLoadHardening)
- addPass(createX86SpeculativeLoadHardeningPass());
+ // Will only run if force enabled or detects the relevant attribute.
+ addPass(createX86SpeculativeLoadHardeningPass());
----------------
I guess this is true for some other passes too, and they don't add such a
comment here. Maybe best to remove this comment if my guess is right?
Repository:
rL LLVM
https://reviews.llvm.org/D51157
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
