[PATCH] D126903: [clang] Add support for __builtin_memset_inline

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: clang/test/Sema/builtins-memcpy-inline.cpp:11
+void test_memcpy_inline_invalid_arg_types() {
+  __builtin_memcpy_inline(1, 2, 3); // expected-error {{cannot initialize a parameter of type 'void *' with an rvalue of type 'int'}}
+}
----------------
can you split this off (and the corresponding code) to a separate patch ?


================
Comment at: llvm/docs/LangRef.rst:13898
+Note that, unlike the standard libc function, the ``llvm.memset.inline.*``
+intrinsics do not return a value, takes extra isvolatile
+arguments and the pointers can be in specified address spaces.
----------------
courbet wrote:
> "an extra `isvolatile` argument"
take*


================
Comment at: llvm/docs/LangRef.rst:13898-13899
+Note that, unlike the standard libc function, the ``llvm.memset.inline.*``
+intrinsics do not return a value, takes extra isvolatile
+arguments and the pointers can be in specified address spaces.
+
----------------
"an extra `isvolatile` argument"


================
Comment at: llvm/docs/LangRef.rst:13899
+intrinsics do not return a value, takes extra isvolatile
+arguments and the pointers can be in specified address spaces.
+
----------------
pointer*


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:993
 
 /// This class wraps the llvm.memset intrinsic.
 class MemSetInst : public MemSetBase<MemIntrinsic> {
----------------
update comment


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:654
 
+// Memset semantic that is guaranteed to be inlined.
+// In particular this means that the generated code is not allowed to call any
----------------
"version" ? or "semantics"


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:7334
   if (TSI) {
     SDValue Result = TSI->EmitTargetCodeForMemset(
         *this, dl, Chain, Dst, Src, Size, Alignment, isVol, DstPtrInfo);
----------------
There is nothing preveting a target from emitting a call to `memset` here when `AlwaysInline` is `true`. Actually, `X86SelectionDAGInfo` does just that (this very patch is adding `/* AlwaysInline */ false,` to the `getMemset` call that handles the trailing bytes). It happens that because trailing bytes are typically small and therefore inline. it happens to work, but this should be verified somehow (or, maybe easier,  `AlwaysInline` should be passed to `EmitTargetCodeForMemset` so it can do the right thing).


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:7344
+    assert(ConstantSize && "AlwaysInline requires a constant size!");
+    return getMemsetStores(*this, dl, Chain, Dst, Src,
+                           ConstantSize->getZExtValue(), Alignment, isVol, true,
----------------
Not that this is not strictly required to return a valid `SDValue`: Even with an "infinite" `Limit` in `TLI.findOptimalMemOpLowering`, a target that overrides this function could decide to just return false. I'm not sure what we want to do in this case. 
So I think we should document `findOptimalMemOpLowering` to mention that the default implementation always returns a valid memop lowering if `Limit` is `UINT_MAX` and that target that decide to not provide a memop lowering *must* emit a valid one in `EmitTargetCodeForMemset`. Another option would be to call the generic `TargetLowering::findOptimalMemOpLowering` when the target declines to generate eithe ra memop lowering or target-specific code.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5928
+    bool isTC = I.isTailCall() && isInTailCallPosition(I, DAG.getTarget());
+    // FIXME: Support passing different dest/src alignments to the memcpy DAG
+    // node.
----------------
memset


================
Comment at: llvm/test/CodeGen/X86/memset-inline.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -mcpu=core2 | FileCheck %s
----------------
Add tests for other targets ?


================
Comment at: llvm/test/CodeGen/X86/memset-inline.ll:21
+define void @regular_memset_calls_external_function(i8* %a, i8 %value) nounwind {
+; CHECK-LABEL: regular_memset_calls_external_function:
+; CHECK:       # %bb.0:
----------------
I could see memset deciding to inline 129 bytes in the future. What about using a more absurdly large number ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126903