sconstab marked 12 inline comments as done.
sconstab added inline comments.
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:79
+
+bool X86LoadValueInjectionIndirectThunksPass::doInitialization(Module &M) {
+ InsertedThunks = false;
----------------
zbrid wrote:
> 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?
As mentioned in a code comment at the beginning of this file, a lot of this
code was lifted from X86RetpolineThunks.cpp, including the logic to insert one
thunk per module. So I didn't actually compose this logic, but my understanding
of it seems to match yours.
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:87
+ STI = &MF.getSubtarget<X86Subtarget>();
+ if (!(STI->hasSSE2() || STI->is64Bit())) {
+ // FIXME: support 32-bit
----------------
zbrid wrote:
> Why is 32-bit okay if it has SSE2 features? (Asking to understand since my
> processor knowledge is a bit weak. Thanks!)
Actually it isn't okay (at this time), so you were correct to point this out.
The thunks right now clearly only support 64-bit. It may be conceivable to
support 32-bit that also has SSE2, but I think this would require some extra
work.
================
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");
----------------
zbrid wrote:
> Should this use R11ThunkName instead of this string literal?
I just removed the assertion because it wasn't really necessary.
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:149
+ // inline.
+ AttrBuilder B;
+ B.addAttribute(llvm::Attribute::NoUnwind);
----------------
zbrid wrote:
> 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?
I do think that these are correct. From the LLVM reference manual, NoUnwind
means that "This function attribute indicates that the function never raises an
exception," which should hold for the thunk. The Naked attribute "disables
prologue / epilogue emission for the function," which is something we would not
want.
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
