[PATCH] D123088: attributes: introduce allockind attr for describing allocator fn behavior

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1576
+     * "new": the function returns a new block of memory, or null if the
+       allocation fails
+     * "resize": the function resizes the block of memory specified by
----------------
I think I'd prefer using `alloc` rather than `new` terminology here. `new` is a C++ism.


================
Comment at: llvm/docs/LangRef.rst:1578
+     * "resize": the function resizes the block of memory specified by
+       ``allocptr`` when successful
+     * "free": the function frees the block of memory specified by ``allocptr``
----------------
I think this needs to be a bit more precise. This function a) return a new block of memory or null, b) if the result is non-null, the memory contents from the start of the new block up to the minimum of the original allocation size and new allocation size match that of the `allocptr` argument and c) the `allocptr` argument is invalidated (in the sense of losing provenance) even if the function returns the same address.


================
Comment at: llvm/docs/LangRef.rst:1580
+     * "free": the function frees the block of memory specified by ``allocptr``
+     * "uninitialized": the function returns uninitialized memory
+     * "zeroed": the function returns zeroed memory
----------------
Can this be used only in conjunction with `new` or also with `resize` (where, in the latter case, it would refer only to memory past the original allocation size).


================
Comment at: llvm/docs/LangRef.rst:1583
+     * "aligned": the function returns memory aligned according to the 
+       ``allocalign`` parameter
+    The first three options are mutually exclusive, and the remaining options
----------------
Can only be used in conjunction with `new` and `resize`, presumably?


================
Comment at: llvm/include/llvm/AsmParser/LLParser.h:23
 #include "llvm/IR/ModuleSummaryIndex.h"
+#include <cstdint>
 #include <map>
----------------
Unnecessary include.


================
Comment at: llvm/include/llvm/Support/CodeGen.h:111
+
+  enum class AllocFnKind : uint64_t {
+    Unknown = 0,
----------------
I don't think CodeGen.h is a good fit for this, this doesn't really have anything to do with codegen. Maybe placing it directly in Attributes.h is fine (we can't have it in MemoryBuiltins.h due to layering).


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:2072
+    } else {
+      return error(KindLoc, "unknown allockind");
+    }
----------------
Include `A` in message? I'd also expect a test case for at least this error condition.


================
Comment at: llvm/lib/IR/Attributes.cpp:458
+    AllocFnKind Kind = getAllocKind();
+    std::vector<std::string> parts;
+    if ((Kind & AllocFnKind::New) != AllocFnKind::Unknown) {
----------------
SmallVector


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123088