This is an archive of the discontinued LLVM Phabricator instance.

[MergeFuncs] Fix callsite attributes in thunk generation
ClosedPublic

Authored by jrkoenig on Sep 2 2015, 5:46 PM.

Details

Summary

This change correctly sets the attributes on the callsites
generated in thunks. This makes sure things such as sret, sext, etc.
are correctly set, so that the call can be a proper tailcall.

Also, the transfer of attributes in the replaceDirectCallers function
appears to be unnecessary, but until this is confirmed it will remain.

Diff Detail

Event Timeline

jrkoenig retitled this revision from to [MergeFuncs] Fix callsite attributes in thunk generation.
jrkoenig updated this object.
jrkoenig added reviewers: jfb, dschuff.
jrkoenig added subscribers: nlewycky, llvm-commits.
jfb added inline comments.Sep 2 2015, 8:53 PM
lib/Transforms/IPO/MergeFunctions.cpp
1558

Why would non-null be missing? Could you add a test that shows this?

I'm not sure I get why the bitcast is needed here, and why you can't just drop the comment. It looks like it's there to match parameters/returns that are layout compatible but have different names. Why not cast those, instead of the function signature?

jrkoenig added inline comments.Sep 3 2015, 11:25 AM
lib/Transforms/IPO/MergeFunctions.cpp
1558

There can be differences between attributes at a callsite and on a function, especially when a call was cross-TU but which is now a static callsite due to linking. The ABI attributes from a callsite and function must match (else the result is UB), and this must already have been true of the input because Old and New have the same ABI attributes. No ABI attributes are transferred in the test suite or in any of my LTO builds, but other attributes are. If transferring other attributes is valuable as an optimization then it should happen in it's own optimization pass, not here.

jfb added inline comments.Sep 3 2015, 1:04 PM
lib/Transforms/IPO/MergeFunctions.cpp
1558

Ah OK, that sounds unfortunate but not worth addressing in the patch. Could you clarify the comment with this?

Clarified comment about questionable attribute transfers.

jfb accepted this revision.Sep 3 2015, 3:34 PM
jfb edited edge metadata.

lgtm

This revision is now accepted and ready to land.Sep 3 2015, 3:34 PM
dschuff added inline comments.Sep 8 2015, 9:50 AM
lib/Transforms/IPO/MergeFunctions.cpp
1555

This comment is a bit confusing as-is because it's not clear what 'This' refers to. Presumably it's the paragraph below; should this paragraph go after the first one instead?

1563

'loose' -> lose

jrkoenig edited edge metadata.

Fixed typo and comment order

This revision was automatically updated to reflect the committed changes.