[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 5 inline comments as done.
chandlerc added a comment.

All outstanding comments addressed, and landing this. Thanks all for the reviews and let me know if I missed anything.



================
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.
----------------
kristof.beyls wrote:
> 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?
Updated the comment.

You can override the compatibility here -- see the `CompatRule` productions just before the `MergeRule` ones.

I'd like this to be documented near the code in addition to in the langref personally. It reminds the reader to look below at the rule sections.


================
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);
+
----------------
kristof.beyls wrote:
> 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?
Long term, no, we don't.

I just have some users that are already passing this flag. Out of convenience, I wanted to leave this working until they have picked up the new version of Clang and LLVM and migrated. I don't expect it to last long (a week or two?).


Repository:
  rL LLVM

https://reviews.llvm.org/D51157