[PATCH] D123090: MemoryBuiltins: start using properties of functions

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In terms of general approach, we'd probably be better off adding support for the attributes in MemoryBuiltins (with a test on a custom allocation function somewhere), and then do the removal from the tables separately (that's also the bit that needs test adjustment).



================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:40
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/CodeGen.h"
 #include "llvm/Support/Debug.h"
----------------
Shouldn't be necessary.


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:106
+  AllocFnKind K = AllocFnKind::Unknown;
+  if (AT & OpNewLike)
+    K |= AllocFnKind::Alloc | AllocFnKind::Uninitialized;
----------------
I'd probably check these with `==` as they are mutually exclusive. Makes it more obvious that you can't end up with a combination from multiple branches here.


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:301
+
+static Optional<StringRef> getAllocationFamilyAttribute(const Value *V) {
+  const auto *CB = dyn_cast<CallBase>(V);
----------------
Isn't this what `getFnAttr()` on CallBase does? That is, return the attribute from either the CallBase or Function?


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:322
+         checkFnAllocKind(V, AllocFnKind::Alloc) ||
+         checkFnAllocKind(V, AllocFnKind::Realloc);
 }
----------------
I think it would be better to fetch the AllocFnKind and then do a single mask check afterwards. We don't want to look up attributes two times.


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:592
+  if (F->hasFnAttribute(Attribute::AllocKind))
+    return AllocFnKind(F->getFnAttribute(Attribute::AllocKind).getValueAsInt());
+  return AllocFnKind::Unknown;
----------------
Same here, this should be only a getFnAttr on CallBase.


================
Comment at: llvm/lib/Transforms/Utils/BuildLibCalls.cpp:1233
   }
+  if (!isLibFreeFunction(&F, TheLibFunc) && !isReallocLikeFn(&F, &TLI))
+    Changed |= setDoesNotFreeMemory(F);
----------------
Maybe add a comment here that we need to do this after allockind has been inferred?


================
Comment at: llvm/test/Transforms/Attributor/heap_to_stack.ll:218
+; IS________NPM-SAME: (i64 [[ALIGNMENT:%.*]]) {
+; IS________NPM-NEXT:    [[TMP1:%.*]] = alloca i8, i64 128, align 1
+; IS________NPM-NEXT:    ret void
----------------
I don't think this should be folding, as the alignment is unknown. Why did this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123090