This is an archive of the discontinued LLVM Phabricator instance.

attributes: introduce allockind attr for describing allocator fn behavior
ClosedPublic

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

Details

Summary

I chose to encode the allockind information in a string constant because
otherwise we would get a bit of an explosion of keywords to deal with
the possible permutations of allocation function types.

I'm not sure that CodeGen.h is the correct place for this enum, but it
seemed to kind of match the UWTableKind enum so I put it in the same
place. Constructive suggestions on a better location most certainly
encouraged.

Diff Detail

Event Timeline

durin42 created this revision.Apr 4 2022, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 3:17 PM
durin42 requested review of this revision.Apr 4 2022, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 3:17 PM
durin42 updated this revision to Diff 420857.Apr 6 2022, 7:41 AM
durin42 edited the summary of this revision. (Show Details)
durin42 updated this revision to Diff 420918.Apr 6 2022, 9:10 AM
durin42 updated this revision to Diff 421210.Apr 7 2022, 7:36 AM
aykevl added a subscriber: aykevl.Apr 12 2022, 3:50 AM

Not a real review but I found this which looks like a bug.

llvm/lib/AsmParser/LLParser.cpp
2061–2062

This looks incorrect to me.

durin42 marked an inline comment as done.Apr 12 2022, 8:32 AM
durin42 added inline comments.
llvm/lib/AsmParser/LLParser.cpp
2061–2062

Good catch, thanks!

durin42 updated this revision to Diff 422245.Apr 12 2022, 8:33 AM
durin42 marked an inline comment as done.
durin42 updated this revision to Diff 424971.Apr 25 2022, 10:55 AM
durin42 updated this revision to Diff 425070.Apr 25 2022, 5:15 PM
durin42 updated this revision to Diff 425364.Apr 26 2022, 5:35 PM
durin42 updated this revision to Diff 425496.Apr 27 2022, 6:05 AM
durin42 updated this revision to Diff 425839.Apr 28 2022, 10:34 AM
nikic added a subscriber: nikic.Apr 29 2022, 1:28 AM
nikic added inline comments.
llvm/docs/LangRef.rst
1576 ↗(On Diff #425839)

I think I'd prefer using alloc rather than new terminology here. new is a C++ism.

1578 ↗(On Diff #425839)

I think this needs to be a bit more precise. This function a) return a new block of memory or null, b) if the result is non-null, the memory contents from the start of the new block up to the minimum of the original allocation size and new allocation size match that of the allocptr argument and c) the allocptr argument is invalidated (in the sense of losing provenance) even if the function returns the same address.

1580 ↗(On Diff #425839)

Can this be used only in conjunction with new or also with resize (where, in the latter case, it would refer only to memory past the original allocation size).

1583 ↗(On Diff #425839)

Can only be used in conjunction with new and resize, presumably?

llvm/include/llvm/AsmParser/LLParser.h
23

Unnecessary include.

llvm/include/llvm/Support/CodeGen.h
111

I don't think CodeGen.h is a good fit for this, this doesn't really have anything to do with codegen. Maybe placing it directly in Attributes.h is fine (we can't have it in MemoryBuiltins.h due to layering).

llvm/lib/AsmParser/LLParser.cpp
2072

Include A in message? I'd also expect a test case for at least this error condition.

llvm/lib/IR/Attributes.cpp
458

SmallVector

durin42 updated this revision to Diff 426087.Apr 29 2022, 9:29 AM
durin42 marked 2 inline comments as done.
durin42 updated this revision to Diff 426095.Apr 29 2022, 9:34 AM
durin42 marked 6 inline comments as done.

I also renamed the enum entry from New to Alloc, hopefully that's what you had in mind.

durin42 updated this revision to Diff 426407.May 2 2022, 7:04 AM
nikic added inline comments.May 2 2022, 12:31 PM
llvm/docs/LangRef.rst
1581 ↗(On Diff #426407)

"string is" -> "string contains"?

1584 ↗(On Diff #426407)

Should reference "alloc" now. Also, I'd suggest to rename resize -> realloc to fully stick with the usual alloc/realloc/free terminology.

1601 ↗(On Diff #426407)

Would it be preferable to forbid them (in the IR verifier) instead?

llvm/include/llvm/Support/CodeGen.h
19

Changes no longer needed.

llvm/lib/IR/Attributes.cpp
461

Can drop the braces from these, as they're single-line.

llvm/test/Assembler/allockind-missing.ll
4 ↗(On Diff #426407)

Oh, fancy. TIL about #@LINE-1.

durin42 updated this revision to Diff 426732.May 3 2022, 8:47 AM
durin42 marked 2 inline comments as done.
durin42 marked 3 inline comments as done.May 3 2022, 8:49 AM
durin42 added inline comments.
llvm/docs/LangRef.rst
1601 ↗(On Diff #426407)

I guess. I've talked myself out of that a few times, because I could envision an allocator where free() needs the alignment passed in for some reason and then it might be nice to mark it as allocalign? I dunno.

I've added the validation now, and tweaked the langref. I guess we can always relax it...

durin42 updated this revision to Diff 426774.May 3 2022, 10:45 AM
nikic added inline comments.May 3 2022, 1:54 PM
llvm/docs/LangRef.rst
1592 ↗(On Diff #426774)

"new" -> "alloc" here

llvm/include/llvm/IR/Attributes.h
49

Space before comment is off here.

llvm/lib/IR/Attributes.cpp
458

Can use StringRef here, as strings are constant?

llvm/lib/IR/Verifier.cpp
2091 ↗(On Diff #426774)

This enumeration of different combinations seems a bit awkward, maybe we can do something along these lines instead?

AllocFnKind Type = K & (AllocFnKind::Alloc | AllocFnKind::Realloc | AllocFnKind::Free);
if (!is_contained(Type, {AllocFnKind::Alloc, AllocFnKind::Realloc, AllocFnKind::Free))
  CheckFailed(...);
2094 ↗(On Diff #426774)

The != AllocFnKind::Unknown checks here aren't really necessary, and imho don't really add to readability either. (K & AllocFnKind::Free) should be directly usable as a boolean. (Though I don't care strongly about this.)

aykevl added inline comments.May 4 2022, 5:06 AM
llvm/docs/LangRef.rst
1584 ↗(On Diff #426774)

Is it worth saying something about alloc(0)? C malloc(0) may or may not return NULL depending on the implementation:

If the size of the space requested is zero, the behavior is implementation-defined: either a null pointer is returned to indicate an error, or the behavior is as if the size were some nonzero value, except that the returned pointer shall not be used to access an object.

nikic added inline comments.May 4 2022, 5:38 AM
llvm/docs/LangRef.rst
1584 ↗(On Diff #426774)

I guess we may want to omit the "if the allocation fails" here (and just make null a valid value -- unless the return is nonnull of course), but I don't think a more specific guarantee on the behavior of zero-sized allocations is needed for the kinds of optimizations we're interested in performing. Or do you have something in mind where this would matter?

For reference, the wording for allocsize is:

This attribute indicates that the annotated function will always return at least a given number of bytes (or null)

It does not further specify in which cases null/non-null may be returned.

durin42 marked 8 inline comments as done.May 4 2022, 11:36 AM
durin42 added inline comments.
llvm/lib/IR/Verifier.cpp
2091 ↗(On Diff #426774)

Very nice, I like it. Thanks!

2094 ↗(On Diff #426774)

It's not: error: could not convert ‘llvm::BitmaskEnumDetail::operator&<llvm::AllocFnKind>(Kind, llvm::AllocFnKind::Free)’ from ‘llvm::AllocFnKind’ to ‘bool’

I think the BitmaskEnum stuff is getting in the way, and it's beyond my C++-fu to figure out how to allow them to be convertible to bool.

durin42 updated this revision to Diff 427088.May 4 2022, 11:36 AM
durin42 marked 2 inline comments as done.
nikic accepted this revision.May 4 2022, 1:12 PM

LGTM

llvm/docs/LangRef.rst
1589 ↗(On Diff #427088)

Missing period at end of sentence.

llvm/include/llvm/Analysis/MemoryBuiltins.h
110

You don't define this function -- or at least not in this patch.

llvm/lib/IR/Verifier.cpp
2094 ↗(On Diff #426774)

Oh yeah, BitmaskEnum is doing some serious magic here...

This revision is now accepted and ready to land.May 4 2022, 1:12 PM
durin42 updated this revision to Diff 427753.May 6 2022, 2:40 PM
durin42 marked 3 inline comments as done.May 6 2022, 2:41 PM
durin42 added inline comments.
llvm/include/llvm/Analysis/MemoryBuiltins.h
110

Oh good catch, this prototype was in the wrong patch. I've moved it to the correct one. Thanks!

durin42 updated this revision to Diff 431696.May 24 2022, 9:45 AM
durin42 marked an inline comment as done.
This revision was landed with ongoing or failed builds.May 31 2022, 7:19 AM
This revision was automatically updated to reflect the committed changes.