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

[Augie Fackler via Phabricator via llvm-commits]

durin42 marked 6 inline comments as done.
durin42 added inline comments.


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:106
+  AllocFnKind K = AllocFnKind::Unknown;
+  if (AT & OpNewLike)
+    K |= AllocFnKind::Alloc | AllocFnKind::Uninitialized;
----------------
bogner wrote:
> nikic wrote:
> > 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.
> They aren't necessarily mutually exclusive though, since `AllocType` has things like `MallocOrCallocLike`, no? 
> 
> However, the current code will set both the `AllocFnKind::Uninitialized` and `AllocFnKind::Zeroed` bits in that case, which seems wrong.
This was correct-enough either way, as this function is only sensible for `AllocType`s that can be set on a function in the table, and MallocOrCallocLike would be nonsensical there.

But it's a little more readable as ==, so I've done that. :)


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:301
+
+static Optional<StringRef> getAllocationFamilyAttribute(const Value *V) {
+  const auto *CB = dyn_cast<CallBase>(V);
----------------
nikic wrote:
> Isn't this what `getFnAttr()` on CallBase does? That is, return the attribute from either the CallBase or Function?
Nope. And fixing that exposed some other weirdness that I no longer remember due to a month of leave, but it pretty rapidly became clear that this was the right fix for now.


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:592
+  if (F->hasFnAttribute(Attribute::AllocKind))
+    return AllocFnKind(F->getFnAttribute(Attribute::AllocKind).getValueAsInt());
+  return AllocFnKind::Unknown;
----------------
nikic wrote:
> Same here, this should be only a getFnAttr on CallBase.
As above, it doesn't work the way you'd expect it to (sigh), and fixing it broke a bunch of unrelated stuff in spooky ways. I'm afraid I don't remember more (no notes, and a month leave erased whatever I did remember).


================
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
----------------
nikic wrote:
> I don't think this should be folding, as the alignment is unknown. Why did this change?
I believe this is a bug I've seen elsewhere in Heap2Stack logic where it sometimes gets over-zealous with unknown alignment and emits incorrect allocas. I agree this shouldn't fold, but I'm not sure how we'd fix that while also letting other optimizations keep working.

See also https://discourse.llvm.org/t/rfc-attributes-for-allocator-functions-in-llvm-ir/61464 for a similar case involving malloc. I think it's probably the same defect (no alignment detected -> alloca with alignment of 1).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123090