This is an archive of the discontinued LLVM Phabricator instance.

MemoryBuiltins: start using properties of functions
ClosedPublic

Authored by durin42 on Apr 4 2022, 3:17 PM.

Details

Summary

Prior to this change, we relied on the hard-coded list for all of the
information performed by MemoryBuiltins. With this change, we're able to
start relying on properites of functions described in attributes, which
opens the door to out-of-tree compilers being able to describe their
allocator functions to LLVM's optimizer logic without having to register
their implementation details with LLVM.

Diff Detail

Event Timeline

durin42 created this revision.Apr 4 2022, 3:17 PM
Herald added a project: Restricted Project. · View Herald Transcript
durin42 requested review of this revision.Apr 4 2022, 3:17 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
durin42 updated this revision to Diff 420859.Apr 6 2022, 7:42 AM
durin42 edited the summary of this revision. (Show Details)
durin42 updated this revision to Diff 420920.Apr 6 2022, 9:10 AM
durin42 updated this revision to Diff 421212.Apr 7 2022, 7:36 AM
durin42 updated this revision to Diff 422247.Apr 12 2022, 8:33 AM
durin42 updated this revision to Diff 424973.Apr 25 2022, 10:55 AM
durin42 updated this revision to Diff 425366.Apr 26 2022, 5:35 PM
durin42 updated this revision to Diff 425498.Apr 27 2022, 6:05 AM
durin42 updated this revision to Diff 425841.Apr 28 2022, 10:34 AM
durin42 updated this revision to Diff 426089.Apr 29 2022, 9:29 AM
durin42 updated this revision to Diff 426097.Apr 29 2022, 9:35 AM
durin42 updated this revision to Diff 426409.May 2 2022, 7:04 AM
durin42 updated this revision to Diff 426734.May 3 2022, 8:47 AM
durin42 updated this revision to Diff 426776.May 3 2022, 10:45 AM
durin42 updated this revision to Diff 427090.May 4 2022, 11:36 AM
durin42 updated this revision to Diff 427755.May 6 2022, 2:40 PM
durin42 updated this revision to Diff 430161.May 17 2022, 12:28 PM
nikic added a comment.Jun 7 2022, 12:49 PM

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).

llvm/lib/Analysis/MemoryBuiltins.cpp
40

Shouldn't be necessary.

105

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.

299

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.

320

Isn't this what getFnAttr() on CallBase does? That is, return the attribute from either the CallBase or Function?

625

Same here, this should be only a getFnAttr on CallBase.

llvm/lib/Transforms/Utils/BuildLibCalls.cpp
1233

Maybe add a comment here that we need to do this after allockind has been inferred?

llvm/test/Transforms/Attributor/heap_to_stack.ll
218 ↗(On Diff #431699)

I don't think this should be folding, as the alignment is unknown. Why did this change?

bogner added a subscriber: bogner.Jun 15 2022, 11:03 AM
bogner added inline comments.
llvm/lib/Analysis/MemoryBuiltins.cpp
105

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.

durin42 marked 6 inline comments as done.Jul 12 2022, 3:47 PM
durin42 added inline comments.
llvm/lib/Analysis/MemoryBuiltins.cpp
105

This was correct-enough either way, as this function is only sensible for AllocTypes 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. :)

320

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.

625

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).

llvm/test/Transforms/Attributor/heap_to_stack.ll
218 ↗(On Diff #431699)

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).

durin42 updated this revision to Diff 444095.Jul 12 2022, 3:47 PM
durin42 marked 3 inline comments as done.

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, then do the removal at the end? I can do that restructuring if that's what you have in mind.

nikic added a comment.Jul 14 2022, 6:58 AM

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, 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.

llvm/lib/Analysis/MemoryBuiltins.cpp
320

I'm confused now. Didn't you already fix this the API in https://github.com/llvm/llvm-project/commit/e90bce8f9191ef1eb2a9b5ff6c90a094acb8de0a?

durin42 updated this revision to Diff 445898.Jul 19 2022, 11:57 AM
durin42 updated this revision to Diff 446190.Jul 20 2022, 9:56 AM
durin42 updated this revision to Diff 446203.Jul 20 2022, 10:35 AM
nikic added inline comments.Jul 20 2022, 1:52 PM
llvm/lib/Analysis/MemoryBuiltins.cpp
103

Function is unused (at least in this patch).

301–302

To avoid duplicate lookup. Same below.

315

It would probably be simpler to just pass in (for example) AllocFnKind::Alloc | AllocFnKind::Realloc and then check that *OAFK & Wanted != 0?

durin42 updated this revision to Diff 446560.Jul 21 2022, 10:24 AM
durin42 edited the summary of this revision. (Show Details)
durin42 marked an inline comment as done.Jul 21 2022, 10:27 AM

Should be ready. For some reason I can't find some of your comments to mark done, but everything should be done (this is essentially the version you shared with me on zulip)

nikic accepted this revision.Jul 21 2022, 12:05 PM

LGTM

This revision is now accepted and ready to land.Jul 21 2022, 12:05 PM
This revision was landed with ongoing or failed builds.Jul 21 2022, 12:31 PM
This revision was automatically updated to reflect the committed changes.