This is an archive of the discontinued LLVM Phabricator instance.

BuildLibCalls: add allocalign attributes for memalign and aligned_alloc
ClosedPublic

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

Details

Summary

This gets us close to being able to remove a column from the table in
MemoryBuiltins.cpp.

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

Question for reviewers: if I can do something similar for the aligned versions of ::operator::new in C++, we can remove a column from the table in MemoryBuiltins.cpp. Should that happen in clang, or someplace else? What am I looking for in clang to make that happen?

durin42 updated this revision to Diff 402906.Jan 25 2022, 7:04 AM
durin42 edited the summary of this revision. (Show Details)
durin42 updated this revision to Diff 404921.Feb 1 2022, 7:00 AM
reames requested changes to this revision.Feb 3 2022, 9:02 AM

Patch appears to be stale, general direction looks reasonable. Marking as require changes to indicate author action needed once dependency lands.

This revision now requires changes to proceed.Feb 3 2022, 9:02 AM
durin42 requested review of this revision.Feb 4 2022, 1:13 PM
durin42 updated this revision to Diff 406091.

Question for reviewers: if I can do something similar for the aligned versions of ::operator::new in C++, we can remove a column from the table in MemoryBuiltins.cpp. Should that happen in clang, or someplace else? What am I looking for in clang to make that happen?

I suggest adding the C++ calls here too, especially if that enables removing the column from MemoryBuiltins, as you suggest.

I think only once we've added all of the attributes required to enable us to eliminate the C++ operators from MemoryBuiltins, then we can delete the special handling of C++ operator new in LLVM. and move all the attribute additions into Clang, all at once. (It just seems simpler to deal with by batching it up that way, instead of moving things across the llvm/clang layer piecemeal).

nikic added a subscriber: nikic.Feb 7 2022, 7:42 AM

Unlike C allocator functions, we do not annotate operator new in SLC. Operator new annotations are left to clang. I believe you should already find handling for aligned new there and just need to switch it to use the new attribute rather than emitting an alignment assumption.

Unlike C allocator functions, we do not annotate operator new in SLC. Operator new annotations are left to clang. I believe you should already find handling for aligned new there and just need to switch it to use the new attribute rather than emitting an alignment assumption.

Except that they _are_ currently special-cased in LLVM, just hardcoded in MemoryBuiltins.cpp vs having attributes defined for the behaviors. It seemed simpler to keep the division of labor between LLVM/Clang, until everything can be moved over at once. But, I don't feel strongly about this -- if you feel the opposite is better, OK.

durin42 updated this revision to Diff 406914.Feb 8 2022, 11:58 AM

Unlike C allocator functions, we do not annotate operator new in SLC. Operator new annotations are left to clang. I believe you should already find handling for aligned new there and just need to switch it to use the new attribute rather than emitting an alignment assumption.

Except that they _are_ currently special-cased in LLVM, just hardcoded in MemoryBuiltins.cpp vs having attributes defined for the behaviors. It seemed simpler to keep the division of labor between LLVM/Clang, until everything can be moved over at once. But, I don't feel strongly about this -- if you feel the opposite is better, OK.

There's new patches in the stack now (annotated correctly in phabricator) that add the correct attribute-adding stuff to clang, and then removes the allocalign column from the table in MemoryBuiltins.cpp. PTAL?

In other words, I should discard this change? Is that right?

nikic added a comment.Feb 10 2022, 9:55 AM

I think you'd want to keep this change. It is still relevant for non-clang frontends with an FFI interface. It would seem silly to not infer one particular attribute here while we infer everything else.

jyknight accepted this revision.Feb 10 2022, 3:24 PM

Agree, we do want this. The general strategy (which I don't think should be changed) is for LLVM to be aware of standard libc functions, and infer attributes for them. (We do want to get it out of the business of being aware of the C++ stdlib functions, however.)

jyknight requested changes to this revision.Feb 10 2022, 3:28 PM
jyknight added inline comments.
llvm/lib/Transforms/Utils/BuildLibCalls.cpp
228

Just noticed: this was supposed to be hasParamAttribute, not hasFnAttribute.

This revision now requires changes to proceed.Feb 10 2022, 3:28 PM
durin42 requested review of this revision.Feb 11 2022, 1:12 PM
durin42 updated this revision to Diff 408017.
nikic accepted this revision.Feb 11 2022, 2:31 PM

LGTM

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

nit: Drop braces for single-line if.

durin42 updated this revision to Diff 408095.Feb 11 2022, 4:15 PM
durin42 updated this revision to Diff 408114.Feb 11 2022, 5:07 PM
durin42 marked 2 inline comments as done.Feb 11 2022, 5:08 PM
durin42 updated this revision to Diff 410056.Feb 18 2022, 7:34 PM
jyknight accepted this revision.Feb 23 2022, 5:38 AM
xbolva00 accepted this revision.Feb 23 2022, 5:55 AM
durin42 updated this revision to Diff 410894.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