This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Simplify calls with casted "returned" attribute
ClosedPublic

Authored by jdoerfert on Apr 12 2020, 10:49 AM.

Details

Summary

The handling of the returned attribute in D75815 did miss the case
where the argument is (bit)casted to a different type. This is
explicitly allowed by the language reference and exposed by the
Attributor.

Diff Detail

Event Timeline

jdoerfert created this revision.Apr 12 2020, 10:49 AM
Herald added a reviewer: uenoku. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
lebedev.ri added inline comments.Apr 12 2020, 12:54 PM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4662

Why only pointers?
https://llvm.org/docs/LangRef.html#parameter-attributes

returned

This indicates that the function always returns the argument as its return value. <...> The parameter and the function return type must be valid operands for the bitcast instruction.
jdoerfert updated this revision to Diff 256892.Apr 12 2020, 4:36 PM

Use canLosslesslyBitCastTo as suggested by @lebedev.ri and test it via vector casts

jdoerfert marked an inline comment as done.Apr 12 2020, 4:36 PM
lebedev.ri accepted this revision.Apr 13 2020, 1:06 AM

LG

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4660

I can't imagine it matters, but these should be swapped around.

This revision is now accepted and ready to land.Apr 13 2020, 1:06 AM
nikic added inline comments.Apr 13 2020, 1:39 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4660

Shouldn't this be an assertion rather than an if check?

lebedev.ri added inline comments.Apr 13 2020, 2:25 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4660

Is there a verifier check for that?

jdoerfert marked an inline comment as done.Apr 13 2020, 8:28 AM
jdoerfert added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4660

@nikic has a point. To fix the issue we just need the CreateBitOrPointerCast, anything that is not valid is an error in the IR.
@lebedev.ri are you sure wrt. the order.We cast the returned argument to the call type so I figured we want to know if the returned argument type can be casted to the call return type.

I think I'll update with an assertion.

xbolva00 added inline comments.
llvm/test/Transforms/InstCombine/call-returned.ll
58

I am wondering if these all cases can be “reversed” to exploit tail call optimization.

lebedev.ri marked an inline comment as done.Apr 13 2020, 10:00 AM
lebedev.ri added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4660

@lebedev.ri are you sure wrt. the order.We cast the returned argument to the call type so I figured we want to know if the returned argument type can be casted to the call return type.

Hm, i guess it depends on how you look at it, either one can be argued to be the right one.

I'd prefer for the current patch to proceed as-is first, and for things to be refactored later.

<jdoerfert> LebedevRI: are u OK with the assertion instead of the conditional?
<LebedevRI> if we want to make that an assertion, we should do it in verifier
<LebedevRI> and avoid assertion there at all
<LebedevRI> (there == in the patch at hand)
<LebedevRI> but both the assertion and verifier may find things, so you might want to proceed with the current patch first and refactor later
<jdoerfert> can u add that comment there please. I'll do that then
This revision was automatically updated to reflect the committed changes.