[PATCH] D59827: [slh] x86 impl of ARM instrinsic for SLH

Kristof Beyls via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 27 07:30:23 PDT 2019


kristof.beyls added a comment.

Thanks for picking this up, Zola!

I quickly looked through the patch - comparing it with what I had done under D49070 <https://reviews.llvm.org/D49070> and D49073 <https://reviews.llvm.org/D49073>.
Apart from the point remarks inline, I had the following immediate thoughts:

1. Could you clang-format the patch?
2. Could you rebase the patch to top-of-trunk (it seems it is a bit behind ToT)?
3. For discussions, seeing the whole patch as it is might be helpful. OTOH, I think it also makes reviewing easier if the target-dependent and the target-independent parts would be split. I think that could also help others in implementing the intrinsics for their targets: they'd have guidance on what might be needed from that target-dependent implementation patches for X86 and AArch64.
4. Lowering to LFENCE seems a correct lowering to me, but someone more knowledgeable about x86 should confirm.
5. I think the LLVM-IR intrinsic should be target-independent, and not x86-specific. That would result in less duplication of code when implementing support for multiple architectures. I seem to remember that's how I implemented this in D49070 <https://reviews.llvm.org/D49070>. I didn't look so far at the SelectionDAG parts of this patch, as I think the differences between my implementation in D49070 <https://reviews.llvm.org/D49070> and this patch may go away after making the intrinsic target-independent.

If we'd take the discussion about adding support for intrinsic `T __builtin_speculation_safe_value(T v)` further here, I'd be happy to abandon the patch series at D49073 <https://reviews.llvm.org/D49073>.
However, in that case, I think the explanation of the intrinsic there should be copied over here to provide a bit more context.



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13
 
+#include "/usr/local/google/home/zbrid/repos/llvm-project/clang/lib/CodeGen/CodeGenTypeCache.h"
 #include "CGCXXABI.h"
----------------
This line doesn't seem to be needed?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3947
+    llvm::Type *T = ConvertType(E->getType());
+    assert((isa<llvm::IntegerType>(T) || isa<llvm::PointerType>(T)) && "unsupported type");
+
----------------
line too long - run clang-format?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1496
+  case Builtin::BI__builtin_speculation_safe_value:
+   return SemaBuiltinSpeculationSafeValueOverloaded(TheCallResult);
   }
----------------
needs one more space of indentation?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5325
+  // Too many args
+  if (TheCall->getNumArgs() < 1)
+    return Diag(TheCall->getEndLoc(), diag::err_typecheck_call_too_many_args_at_most)
----------------
Should this be "TheCall->getNumArgs() > 1" (larger than rather then less than)?


================
Comment at: clang/test/CodeGen/builtin-speculation-safe-value.c:1-2
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm %s -o - | FileCheck -check-prefix=CHECK-SUPPORTED %s
+
----------------
When I wrote this test in D49073 this line read "REQUIRES: aarch64-registered-target". Looking at this now, I wonder why the requires might be needed, beyond the RUN line containing "-triple x86_64-linux-gnu". It'd be nice if this test didn't need a REQUIRES line.... But maybe there is a good reason it does need a requires line after all?


================
Comment at: clang/test/Preprocessor/init.c:9215
 // WEBASSEMBLY-NEXT:#define __GXX_ABI_VERSION 1002
+// WEBASSEMBLY-NEXT:#define __HAVE_SPECULATION_SAFE_VALUE 1
 // WEBASSEMBLY32-NEXT:#define __ILP32__ 1
----------------
It seems this is the only intended change in this file; all the other changes in this file were unintended for this patch?



================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1171
                              [IntrNoMem, Returned<0>]>;
+
 //===----------------------------------------------------------------------===//
----------------
accidental new line diff?


================
Comment at: llvm/include/llvm/IR/IntrinsicsX86.td:4819-4822
+
+//===----- Intrinsics to mitigate against miss-speculation exploits -------===//
+
+def int_speculationsafevalue : Intrinsic<[llvm_any_ty], [LLVMMatchType<0>], []>;
----------------
I think this needs to be a target independent LLVM IR intrinsic, not x86 specific. See D49070. This will also need documentation in LangRef.rst then, also see D49070 for a possible documentation I proposed for this intrinsic there.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:614-628
+      if (Opcode == X86::SpeculationSafeValue32) {
+        BuildMI(MBB, NMBBI, DebugLoc(), TII->get(X86::LFENCE));
+        ++NumInstsInserted;
+        ++NumLFENCEsInserted;
+        MRI->replaceRegWith(MI.getOperand(0).getReg(), MI.getOperand(1).getReg());
+        MI.eraseFromParent();
+        Modified = true;
----------------
The lowering of the intrinsic on a 32 bit and a 64 bit value looks identical to me, so the if statement isn't needed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59827/new/

https://reviews.llvm.org/D59827





More information about the cfe-commits mailing list