Page MenuHomePhabricator

Attributes: add a new allocalign attribute
ClosedPublic

Authored by durin42 on Jan 21 2022, 12:53 PM.

Details

Summary

This will let us start moving away from hard-coded attributes in
MemoryBuiltins.cpp and put the knowledge about various attribute
functions in the compilers that emit those calls where it probably
belongs.

Diff Detail

Event Timeline

durin42 created this revision.Jan 21 2022, 12:53 PM
durin42 requested review of this revision.Jan 21 2022, 12:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 12:53 PM
nikic added a reviewer: nikic.Jan 21 2022, 1:01 PM
nikic requested changes to this revision.Jan 21 2022, 1:11 PM

Might it make more sense to implement this as an argument attribute? Would be a bit simpler I think, and I don't really see a disadvantage.

If you do want this to be a function int attribute, then the parameter reference should be zero-based, not one-based, for consistency with allocsize.

This revision now requires changes to proceed.Jan 21 2022, 1:11 PM
reames requested changes to this revision.Jan 21 2022, 1:14 PM

Cannot review without LangRef changes.

ormris removed a subscriber: ormris.Jan 24 2022, 11:07 AM

Might it make more sense to implement this as an argument attribute? Would be a bit simpler I think, and I don't really see a disadvantage.

I don't have any idea. @reames any opinion? I confess I'm not sure what this means for me.

If you do want this to be a function int attribute, then the parameter reference should be zero-based, not one-based, for consistency with allocsize.

I can't do it zero-based, because there's no way to store a zero in an int attribute. I can go to the work of adding getters and setters everywhere and then pretend it's 0-based for the public API, but internally you'll fail assertions if you try to carry 0 as the payload of an int attr. If you want the getters and setters let me know, I guess? This will matter for one other attribute we need too (mustfree(N) - to indicate a pointer will be freed).

Cannot review without LangRef changes.

Done.

durin42 requested review of this revision.Jan 25 2022, 7:04 AM
durin42 updated this revision to Diff 402904.
durin42 edited the summary of this revision. (Show Details)

Might it make more sense to implement this as an argument attribute? Would be a bit simpler I think, and I don't really see a disadvantage.

I don't have any idea. @reames any opinion? I confess I'm not sure what this means for me.

I don't see anyway with argument align attributes to express the alignment of the return value of the call. Specifically, the fact this is trying to express is "the returned pointer has at least the alignment of the integer param X".

If you do want this to be a function int attribute, then the parameter reference should be zero-based, not one-based, for consistency with allocsize.

I can't do it zero-based, because there's no way to store a zero in an int attribute. I can go to the work of adding getters and setters everywhere and then pretend it's 0-based for the public API, but internally you'll fail assertions if you try to carry 0 as the payload of an int attr. If you want the getters and setters let me know, I guess? This will matter for one other attribute we need too (mustfree(N) - to indicate a pointer will be freed).

Er, this seems strongly suspect to me. I have not looked carefully, but I'm pretty sure we do use zero based indexing in allocsize. You should be able to parallel that structure. (You don't need the argument packing bit, but at the same time it does no harm to reuse if it simplifies the code.)

You need tests for this. Both IR, and bitcode parsing.

I would suggest focusing this change on the mechanics of round tripping the new attribute and defining it. We can defer the use to a separate patch.

llvm/docs/LangRef.rst
1556

This definition needs expanded.

Questions:

  • Units?
  • one argument, two arguments?
  • assumptions about new memory?

See the alllocsize for example text.

llvm/lib/Analysis/MemoryBuiltins.cpp
227 ↗(On Diff #402904)

Nope, would be same place we handle allocsize today.

339 ↗(On Diff #402904)

To parallel the allocsize path, we need builtin knowledge to overrule attributed knowledge.

reames requested changes to this revision.Jan 25 2022, 7:25 AM
This revision now requires changes to proceed.Jan 25 2022, 7:25 AM

If you do want this to be a function int attribute, then the parameter reference should be zero-based, not one-based, for consistency with allocsize.

I can't do it zero-based, because there's no way to store a zero in an int attribute. I can go to the work of adding getters and setters everywhere and then pretend it's 0-based for the public API, but internally you'll fail assertions if you try to carry 0 as the payload of an int attr. If you want the getters and setters let me know, I guess? This will matter for one other attribute we need too (mustfree(N) - to indicate a pointer will be freed).

Er, this seems strongly suspect to me. I have not looked carefully, but I'm pretty sure we do use zero based indexing in allocsize. You should be able to parallel that structure. (You don't need the argument packing bit, but at the same time it does no harm to reuse if it simplifies the code.)

allocsize is different because it has two parameters, one optional. It's not possible for both to be set to zero, so the payload it carries will _never_ be zero. It does use zero-based indexing, but we'll have to do custom getters/settings across the board like allocsize if we want to use zero-based indexing in both allocalign and mustfree. If that's the preference, I'm happy to do it, but it does seem like a fair amount of extra code for only marginal benefit.

nikic added a comment.Jan 25 2022, 7:56 AM

Might it make more sense to implement this as an argument attribute? Would be a bit simpler I think, and I don't really see a disadvantage.

I don't have any idea. @reames any opinion? I confess I'm not sure what this means for me.

I don't see anyway with argument align attributes to express the alignment of the return value of the call. Specifically, the fact this is trying to express is "the returned pointer has at least the alignment of the integer param X".

To clarify, what I mean is:

; Instead of
declare i8* @aligned_alloc(i64, i64) allocalign(1)
; You do
declare i8* @aligned_alloc(i64, i64 allocalign)

That is, the allocalign attribute indicates that the value provided to this parameter will the alignment of the return value. The semantics would be the same as with the return attribute, you just don't need to deal with integer attributes.

If we do want to use the function attribute, I think it should be possible to support proper zero values for integer attributes.

That is, the allocalign attribute indicates that the value provided to this parameter will the alignment of the return value. The semantics would be the same as with the return attribute, you just don't need to deal with integer attributes.

Using a parameter attribute instead of a function attribute with a parameter number seems quite reasonable. Let's go with that.

If we do want to use the function attribute, I think it should be possible to support proper zero values for integer attributes.

I don't think this would be straightforward. The attribute system has the assumption that "unset == 0" in many parts of its API.

jyknight added inline comments.Jan 25 2022, 3:03 PM
llvm/docs/LangRef.rst
1556

I don't think the allocsize description is a great example of clarity...

For allocsize, I'd want to know how it relates to -- or differs from -- placing dereferenceable_or_null(value-of-EltSizeParam) or dereferenceable_or_null(value-of-EltSizeParam * value-of-NumEltsParam) on the function return. Other than that the latter is only expressible when the values are constants, of course.

It is not at all clear from the wording, but it's treated completely differently internally right now. (Maybe it shouldn't actually be? Dunno! The current description doesn't seem to suggest any of the semantics we've actually applied to it!)

Anyways, for allocalign, I think we can probably do better.

E.g, maybe we can say "This attribute is equivalent to placing align(value-of-allocalign-parameter) on the function return, except that the value is not required to be a constant, and <something about non-power-of-2 values>." -- and actually have the code treat it that way, as well.

So, what _should_ the semantics be if the parameter is not a power-of-2?

  • UB.
  • Ignore (treat as if alignment was unknown -- e.g. 1)
  • Something else?

Looks like we're currently inconsistent in the handling at the moment for the hardcoded functions. AttributorAttributes.cpp seems to be assuming power-of-2 (and tries to create an invalid alloca instruction if it's not), while InstCombineCalls.cpp ignores the specified alignment if it's not a power-of-2. (Am I the only one surprised that "Attributor" can make very-much non-attribute IR changes like doing heap-to-stack!)

I think I'd vote for "ignore".

Units: Yea, clearly this is "alignment in bytes", but that should be specified.

llvm/lib/Analysis/MemoryBuiltins.cpp
339 ↗(On Diff #402904)

Why does allocsize work that way? I'd generally expect an attribute to take precedence if specified?

durin42 marked 5 inline comments as done.Jan 26 2022, 9:14 AM
durin42 added inline comments.
llvm/lib/Analysis/MemoryBuiltins.cpp
339 ↗(On Diff #402904)

I'm new to compilers at all and LLVM in particular, but I really would have expected an attribute to overrule builtin knowledge too, to the point that I didn't check how allocsize was set up. For now I'm making it consistent, but it does seem backwards.

Also, the "use the attribute" part of this has moved to a new change.

durin42 requested review of this revision.Jan 26 2022, 9:14 AM
durin42 updated this revision to Diff 403306.
durin42 retitled this revision from Attributes: add a new allocalign() attribute to Attributes: add a new allocalign attribute.
durin42 marked an inline comment as done.

It does use zero-based indexing, but we'll have to do custom getters/settings across the board like allocsize if we want to use zero-based indexing in both allocalign and mustfree. If that's the preference, I'm happy to do it, but it does seem like a fair amount of extra code for only marginal benefit.

If the existing infrastructure truly doesn't allow zero as a value for an attribute as an expressability level, then it's over specialized for existing attributes, and we should generalize.

With @nikic's clarification, I now understand what he meant, and agree that looks reasonable as well.

llvm/lib/Analysis/MemoryBuiltins.cpp
339 ↗(On Diff #402904)

I have no idea why the code is structured the current way, but having the two *differ* seems like a recipe for confusion and disaster.

If you want to change the way the existing attribute is handled, you could do the research and work through that.

If you want to leave the order the same for the two, and add a TODO for latter work on reversing it, I'm also fine. Note that order doesn't matter if the baseline knowledge is removed as we seem to be working towards.

nikic added inline comments.Jan 27 2022, 1:02 PM
llvm/docs/LangRef.rst
1554

This shouldn't be in the "function attributes" section.

1555

"The function parameter marked with this attribute is [...]" maybe? The current phrasing reads odd to me.

1556

I would probably go for UB here, in the interest of allowing handling of non-constant alignments by creating assumes. LangRef is not clear on this, but I expect that an align operand bundle assume with a non-power-of-two value would be UB.

No strong preference though.

durin42 marked 4 inline comments as done.Feb 1 2022, 7:00 AM
durin42 updated this revision to Diff 404920.Feb 1 2022, 7:00 AM
reames requested changes to this revision.Feb 3 2022, 8:56 AM

This patch looks mechanically fine, we just need to get the LangRef wording finalized.

I did have a thought when reading over this. Is there anything specific here to an allocation function? Could we generalize this into a generic "retalign" parameter attribute which generalizes align(N) on the return value and allows non-constant alignments to be described?

I'm not sure that generalization is actually useful, it just seems like we have something which is almost non-allocation specific. If we could think of examples where non-constant alignments are useful, then it seems a shame to leave it specialized...

llvm/docs/LangRef.rst
1383

I can't make heads or tails of the current wording. I'd suggest something along the lines of:

This attribute indicates that the alignment of the newly allocated object returned by this allocation function can be assumed to be greater than the runtime value of the marked parameter.

That is, it's similar to putting`align(N)` on the return value of the function but allows the alignment to be a runtime value.

Runtime alignment values which aren't a power of 2 are undefined behavior.

This revision now requires changes to proceed.Feb 3 2022, 8:56 AM

So, sorry for not noticing this sooner, but...I just realized we actually already have a way to spell something very much like this already, with llvm.assume.

%ret = call i8* @aligned_alloc(i64 %0, i64 %1)
call void @llvm.assume(i1 true) [ "align"(i8* %ret, i64 %0) ]

Maybe we _shouldn't_ be adding an attribute for this, given the above, and rather just insert llvm.assume in appropriate places? @jdoerfert maybe has an opinion here?

llvm/docs/LangRef.rst
1556

On second look, this must NOT be UB.

Per C2x aligned_alloc:
"If the value of alignment is not a valid alignment supported by the implementation the function shall fail by returning a null pointer." (and elsewhere it says "Every valid alignment value shall be a nonnegative integral power of two.")

We cannot turn a guarantee of returning nullptr into UB. We must be prepared to accept any alignment at runtime, even if we ignore non-power-of-2 ones.

nikic added a comment.Feb 3 2022, 12:50 PM

This patch looks mechanically fine, we just need to get the LangRef wording finalized.

I did have a thought when reading over this. Is there anything specific here to an allocation function? Could we generalize this into a generic "retalign" parameter attribute which generalizes align(N) on the return value and allows non-constant alignments to be described?

I'm not sure that generalization is actually useful, it just seems like we have something which is almost non-allocation specific. If we could think of examples where non-constant alignments are useful, then it seems a shame to leave it specialized...

I agree that the actual semantics here should be independent of whether the function is also, in some sense, considered an "allocator". I'd be fine still calling it allocalign in that case (similar to how allocsize is a general statement that the number of bytes is dereferenceable_or_null). retalign also sounds reasonable though.

llvm/docs/LangRef.rst
1556

Good point. In that case I'm fine with saying that non-power-of-two alignment is ignored. Alternatively we could specify that ptrtoint(ret) % param == 0, which would be the non-power-of-two generalization (and trivially holds for the null pointer), but I don't think there is anything useful we could do with that, and it's probably best to stick with the well-defined notion of alignment (don't want to bring question like "what about non-integral pointers?" into this.)

So, sorry for not noticing this sooner, but...I just realized we actually already have a way to spell something very much like this already, with llvm.assume.

%ret = call i8* @aligned_alloc(i64 %0, i64 %1)
call void @llvm.assume(i1 true) [ "align"(i8* %ret, i64 %0) ]

Maybe we _shouldn't_ be adding an attribute for this, given the above, and rather just insert llvm.assume in appropriate places? @jdoerfert maybe has an opinion here?

After thinking about this a bit more, I think I've actually convinced myself that introducing the attribute is the right thing to do after all. We'll need to make sure to document this well, of course...

The semantics of llvm.assume is only that the return value must be aligned _at least_ to the given amount. If you see a @llvm.assume(i1 true) [ "align"(i8* %ret, i64 2) ] that means you know that %ret is aligned to at least 2-bytes. However, "an alignment assumption which is true at this point" is insufficient to be able to correctly do heap->stack conversion of an allocation call. For that, we must know the alignment which was required of that allocation call.

It's semantically correct to lower the alignment value on -- or entirely discard -- an llvm.assume "align" value, but it is NOT valid to use a lower alignment than was requested for an allocation call.

I think, thus, we can say that "allocalign" implies the llvm.assume "align", in terms of what a caller can assume about the return value's alignment, but also has the additional semantics of indicating the requested alignment of the allocation, which the compiler may use if it decides to elide an allocation. (This also implies that generalizing this attribute to "retalign" is not desirable nor necessary.)

durin42 requested review of this revision.Feb 4 2022, 1:13 PM
durin42 updated this revision to Diff 406090.
durin42 updated this revision to Diff 406913.Feb 8 2022, 11:58 AM
jyknight accepted this revision.Feb 10 2022, 3:23 PM

I'm happy with the LangRef semantics and wording that here now.

nikic added inline comments.Feb 11 2022, 2:30 PM
llvm/docs/LangRef.rst
1388

Should we specify that the attribute may only be applied to integer parameters, and add a corresponding rule to typeIncompatible()?

llvm/lib/Transforms/Utils/CodeExtractor.cpp
903

This should be in the bottom category (non-function attrs).

durin42 updated this revision to Diff 408094.Feb 11 2022, 4:15 PM
durin42 updated this revision to Diff 408113.Feb 11 2022, 5:07 PM
durin42 marked 2 inline comments as done.Feb 11 2022, 5:08 PM
durin42 added inline comments.
llvm/docs/LangRef.rst
1388

Sure, done.

llvm/lib/Transforms/Utils/CodeExtractor.cpp
903

Fixed, thanks.

nikic accepted this revision.Feb 12 2022, 12:53 AM

LGTM

durin42 updated this revision to Diff 410055.Feb 18 2022, 7:34 PM
durin42 marked 2 inline comments as done.
jyknight accepted this revision.Feb 23 2022, 5:39 AM
durin42 updated this revision to Diff 410893.Feb 23 2022, 11:58 AM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 4 2022, 1:01 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2022, 1:01 PM