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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D123090#3646751 <https://reviews.llvm.org/D123090#3646751>, @durin42 wrote:

> In D123090#3564542 <https://reviews.llvm.org/D123090#3564542>, @nikic wrote:
>
>> 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).
>
> IOW: implement everything including D123091 <https://reviews.llvm.org/D123091>, then do the removal at the end? I can do that restructuring if that's what you have in mind.

Yeah, that's what I had in mind.



================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:301
+
+static Optional<StringRef> getAllocationFamilyAttribute(const Value *V) {
+  const auto *CB = dyn_cast<CallBase>(V);
----------------
durin42 wrote:
> 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.
I'm confused now. Didn't you already fix this the API in https://github.com/llvm/llvm-project/commit/e90bce8f9191ef1eb2a9b5ff6c90a094acb8de0a?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123090