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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. | |
106 | 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. | |
301 | Isn't this what getFnAttr() on CallBase does? That is, return the attribute from either the CallBase or Function? | |
322 | 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. | |
591 | Same here, this should be only a getFnAttr on CallBase. | |
llvm/lib/Transforms/Utils/BuildLibCalls.cpp | ||
1245 | Maybe add a comment here that we need to do this after allockind has been inferred? | |
llvm/test/Transforms/Attributor/heap_to_stack.ll | ||
262 | I don't think this should be folding, as the alignment is unknown. Why did this change? |
llvm/lib/Analysis/MemoryBuiltins.cpp | ||
---|---|---|
106 | 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. |
llvm/lib/Analysis/MemoryBuiltins.cpp | ||
---|---|---|
106 | 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. :) | |
301 | 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. | |
591 | 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 | ||
262 | 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). |
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 | ||
---|---|---|
301 | I'm confused now. Didn't you already fix this the API in https://github.com/llvm/llvm-project/commit/e90bce8f9191ef1eb2a9b5ff6c90a094acb8de0a? |
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)
Shouldn't be necessary.