Page MenuHomePhabricator

[InstCombine] Fix mismatched attribute lists for combined calls
ClosedPublic

Authored by guiand on Jun 29 2020, 4:16 PM.

Details

Summary

Currently, a transformation like pow(2.0, x) -> exp2(x) copies the pow
attribute list verbatim and applies it to exp2. This works out fine
when the attribute list is empty, but when it isn't clang will error due
due to the mismatch.

This was discovered after implementing the noundef param attribute.

Diff Detail

Event Timeline

guiand created this revision.Jun 29 2020, 4:16 PM
Herald added a project: Restricted Project. · View Herald Transcript
vitalybuka added inline comments.Jun 29 2020, 6:05 PM
llvm/include/llvm/IR/Attributes.h
691 ↗(On Diff #274279)

Is this

AttributeList LdexpAttrs = Attrs.shuffle<-1, 0, 1, 1>(B.getContext());

about the same as

AttributeList LdexpAttrs;
LdexpAttrs.addAttributes(Ctx, -1, Attrs.getAttributes(-1))
LdexpAttrs.addAttributes(Ctx, 0, Attrs.getAttributes(0))
LdexpAttrs.addAttributes(Ctx, 1, Attrs.getAttributes(1))
LdexpAttrs.addAttributes(Ctx, 2, Attrs.getAttributes(1))

I think later is easier to read than format.
maybe as a for loop

Currently, a transformation like pow(2.0, x) -> exp2(x) copies the pow attribute list verbatim and applies it to exp2

For the record, this is clearly wrong even before the noundef patch. Thanks for fixing this.

I let you hash out the details of the shuffle thing ;)

(Ignore the libomp test failures)

lebedev.ri added inline comments.
llvm/include/llvm/IR/Attributes.h
691 ↗(On Diff #274279)

I'm also worried about readability here.

eugenis added inline comments.Jun 30 2020, 12:58 PM
llvm/include/llvm/IR/Attributes.h
691 ↗(On Diff #274279)

Agreed. I like the flexibility of Vitaly's example in case you need to go beyond a simple shuffle.

guiand updated this revision to Diff 274599.Jun 30 2020, 1:12 PM

Addressed comments

eugenis added inline comments.Jun 30 2020, 4:25 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1459

When transforming pow(exp(x), y) to exp(x *y), why is it the right behavior to copy the attributes from the "y" parameter, and drop the ones on "x" and on "exp(x)"? Why not union or intersection?

It feels like the safe default behavior should be to drop all attributes, and adds the ones that we know apply here, like "noundef".

guiand updated this revision to Diff 274906.Jul 1 2020, 1:34 PM

Addressed comments.

guiand marked 2 inline comments as done.Jul 1 2020, 1:36 PM
guiand added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1459

You're definitely right, in the case of combining the pow parameters it doesn't really make sense to assign attributes from one or the other... maybe intersection makes sense though. I do still think it makes sense to copy attributes from x when we're eliminating a constant, like in pow(10.0, x) -> exp10(x). So I've split them up into two different attribute lists.

guiand updated this revision to Diff 274957.Jul 1 2020, 4:47 PM
guiand marked an inline comment as done.

Fix oversight in previous change; pow(2.0**x) -> exp2 adds proper attributes now.

LGTM

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1810

does this assign attributes of x to the 1.0 constant?
why?

guiand added a comment.Tue, Jul 7, 9:48 AM

I was discussing with @eugenis about this and he suggested that it's reasonable not to copy any call-inst attributes when simplifying LibCalls. Specifically because you run into these messy questions of how reasonable it is to blindly apply attributes as a caller. Since LibCalls have their own inherent attribute lists which get applied upon creating a Function instance for them, it seems like we shouldn't impose any further attributes.

This code generates a libcall out of thin air. My intuition says the safest thing to do is to drop all call site attributes, because they generally specify something about how an attribute must be passed to the callee, and not a property of the value being passed, so there is no reason for the attribute lists on pow and on exp to have anything in common at all.

This way we would lose the noundef attribute on the exp call arguments. We might extend TargetLibraryInfo in the future to specify attributes on the declarations.

WDYT?

guiand updated this revision to Diff 276207.Tue, Jul 7, 2:22 PM

This update removes attribute list copying per the discussion above.

efriedma accepted this revision.Tue, Jul 7, 2:28 PM

LGTM

This revision is now accepted and ready to land.Tue, Jul 7, 2:28 PM
vitalybuka accepted this revision.Tue, Jul 7, 2:33 PM
eugenis added inline comments.Tue, Jul 7, 2:39 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1788

This comment seems wrong. (intrinsics have attribute lists, libcalls don't)

guiand updated this revision to Diff 276491.Wed, Jul 8, 10:52 AM
guiand edited the summary of this revision. (Show Details)

Reworded comments to more closely match the reasoning

eugenis accepted this revision.Wed, Jul 8, 11:58 AM

LGTM

@eugenis and I discussed the possibility that removing attributes like this could cause problems if, for some reason, the ABI of a libcall changes as a result. The test I added actually demonstrates this: the inreg attribute is removed from the calls, which could change the behavior.

But this is only a concern if there's some reason to specify ABI-changing attributes to libcalls in the callsite like this, and not have LLVM put them in the declaration. Which I'm not sure there is. What are peoples' thoughts?

If a libcall requires some ABI-changing attribute, presumably we'd make emitBinaryFloatFnCall take care of it, or something like that. I don't think float libcalls currently require ABI attributes on any target, but if they did, there isn't any reason to expect the attributes to be related.

guiand added a comment.Wed, Jul 8, 1:18 PM

Good to hear. Should this be good to go in that case?

This revision was automatically updated to reflect the committed changes.