This is an archive of the discontinued LLVM Phabricator instance.

getAllocAlignment: respect allocalign attribute if present
ClosedPublic

Authored by durin42 on Jan 26 2022, 9:14 AM.

Details

Summary

As with allocsize(), we prefer the table data to attributes.

Diff Detail

Event Timeline

durin42 created this revision.Jan 26 2022, 9:14 AM
durin42 requested review of this revision.Jan 26 2022, 9:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2022, 9:14 AM
durin42 updated this revision to Diff 404922.Feb 1 2022, 7:00 AM
durin42 edited the summary of this revision. (Show Details)
reames requested changes to this revision.Feb 3 2022, 9:01 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:01 AM
durin42 requested review of this revision.Feb 4 2022, 1:13 PM
durin42 updated this revision to Diff 406092.
jyknight added inline comments.Feb 10 2022, 3:41 PM
llvm/lib/Analysis/MemoryBuiltins.cpp
333

Need to update the docs for the function to mention that it uses the alloc_align attribute. Also mention that it can return invalid alignments and callers need to be able to handle that.

And, then, need to actually verify the existing callers of this function can accept an invalid alignment value, since now we're providing one. But in fact, they cannot (as per previous discussion).

That is, llvm/lib/Transforms/IPO/AttributorAttributes.cpp will try to make an alloca instruction with an invalid alignment, which is not permitted.

Basically, the code:

if (Value *Align = getAllocAlignment(AI.CB, TLI)) {
  if (!getAPInt(A, *this, *Align)) {
    [[error case]]

needs to also verify against MaximumAlignment and power-of-2ness, just like the other callsite, in llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp already does.

Probably good to add a helper function, for getting a constant alignment value, e.g. as a Optional<uint64_t>, which calls this function then verifies the result is valid before returning.

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

(Ah, I see, this fix landed in the wrong diff in the stack -- should've been in previous).

durin42 marked 2 inline comments as done.Feb 11 2022, 1:10 PM
durin42 added inline comments.
llvm/lib/Analysis/MemoryBuiltins.cpp
333

Updated the docs that it uses allocalign, but I'm not clear what wording I should be putting on the function about invalid alignment values: this function doesn't return the alignment, but the argument used for alignment. AttributorAttributes.cpp was broken before my change was written!

Can you elaborate on what warning label you want on this function WRT the validation callers should do? I can add it as a later patch as a cleanup while I'm here (but I'm not going to go fix AttributorAttributes right now since it's been broken all along...)

durin42 updated this revision to Diff 408018.Feb 11 2022, 1:12 PM
durin42 marked an inline comment as done.
jyknight added inline comments.Feb 11 2022, 2:14 PM
llvm/lib/Analysis/MemoryBuiltins.cpp
333

"Note: the Value returned may not indicate a valid alignment, per the definition of the allocalign attribute."

I do think it's on you to fix this, despite it being broken before for certain hardcoded known functions, because _now_ you've defined a brand-new attribute which supposedly has certain semantics, yet the code doesn't honor those semantics. That's not really OK, regardless of the state of things for the hardcoded alloc_aligned/memalign functions.

durin42 updated this revision to Diff 408096.Feb 11 2022, 4:15 PM
durin42 updated this revision to Diff 408115.Feb 11 2022, 5:07 PM
durin42 marked an inline comment as done.Feb 11 2022, 5:10 PM
durin42 added inline comments.
llvm/lib/Analysis/MemoryBuiltins.cpp
333

Doc added, and follow-up for the bug that predates me in D119604 (I can reorder them if you'd like, I just don't really trust moz-phab to handle inserting a patch in the middle of the stack correctly...)

nikic added a subscriber: nikic.Feb 12 2022, 12:57 AM
nikic added inline comments.
llvm/lib/Analysis/MemoryBuiltins.cpp
340

Stray semicolon

343

You should use hasAttrSomewhere here (it's more efficient if the attribute is not present anywhere).

durin42 updated this revision to Diff 410057.Feb 18 2022, 7:34 PM
durin42 marked an inline comment as done.
durin42 marked 2 inline comments as done.Feb 18 2022, 7:34 PM
durin42 updated this revision to Diff 410557.Feb 22 2022, 9:02 AM
jyknight accepted this revision.Feb 23 2022, 5:52 AM
durin42 updated this revision to Diff 410895.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