Page MenuHomePhabricator

[BuildLibCalls] Properly set ABI attributes on arguments
ClosedPublic

Authored by aeubanks on Mon, May 31, 9:24 AM.

Details

Summary

Some floating point lib calls have ABI attributes that need to be set on
the caller. Found via D103412.

Diff Detail

Event Timeline

aeubanks created this revision.Mon, May 31, 9:24 AM
aeubanks requested review of this revision.Mon, May 31, 9:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 31, 9:24 AM

this is kinda ugly, but I'm not sure of a nicer way to do it. Maybe a new method on AttributeList that merges attributes?
ldexp() and friends are the only functions that have ABI attributes set (setSignExtendedArg())

rnk added a comment.Wed, Jun 2, 2:04 PM

I'd suggest something like:

CI->setAttributes(CI->getCalledFunction()->getAttributes().removeAttributes(Ctx, llvm::AttributeList::FunctionIndex)));

That should take the callee's attribute list, throw away all function index attributes (things like SSE level, noreturn, noinline, alwaysinline, optnone), and apply the return and parameter attributes. It should also create fewer immortal, immutable, but temporary attribute lists.

I'd suggest something like:

CI->setAttributes(CI->getCalledFunction()->getAttributes().removeAttributes(Ctx, llvm::AttributeList::FunctionIndex)));

That should take the callee's attribute list, throw away all function index attributes (things like SSE level, noreturn, noinline, alwaysinline, optnone), and apply the return and parameter attributes. It should also create fewer immortal, immutable, but temporary attribute lists.

The problem is that right above, we already call

CI->setAttributes()

and a second one would overwrite that.

I was thinking of adding something new to merge two AttributeLists, but idk

rnk added a comment.Wed, Jun 2, 2:28 PM

You're right, I don't see a way to directly merge two AttributeList objects. I looked at AttrBuilder::merge, but AttrBuilder makes AttributeSet objects. I guess it makes sense to have an AttributeList method to do this.

aeubanks updated this revision to Diff 349663.Thu, Jun 3, 1:00 PM

add merge function

aeubanks edited the summary of this revision. (Show Details)Thu, Jun 3, 1:01 PM
rnk accepted this revision.Thu, Jun 3, 2:10 PM

lgtm

There's some horrifying index manipulation involving unsigned overflow that I had to avoid by checking if either AttributeList is empty.

By horrifying, surely you mean far too clever for anyone's best interests. =D

It'd be nice to not have AttributeList::AttrIndex::FunctionIndex = ~0U.

I had a patch to do that back in 2017, but the reviewers at the time felt it was too risky:
https://reviews.llvm.org/D32819#749163
https://reviews.llvm.org/D32819?id=98222#inline-286125

We could try again, but it has the potential to break frontends in ways that are hard to debug.

This revision is now accepted and ready to land.Thu, Jun 3, 2:10 PM
aeubanks updated this revision to Diff 349693.Thu, Jun 3, 2:27 PM

use existing AttributeList merge method
I completely missed that this one already existed

aeubanks edited the summary of this revision. (Show Details)Thu, Jun 3, 2:27 PM
rnk accepted this revision.Thu, Jun 3, 3:10 PM

lgtm

This revision was landed with ongoing or failed builds.Thu, Jun 3, 3:45 PM
This revision was automatically updated to reflect the committed changes.