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