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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
