Page MenuHomePhabricator

[AArch64] Lower calls with rv_marker attribute .
ClosedPublic

Authored by fhahn on Dec 3 2020, 4:19 AM.

Details

Summary

This patch adds support for lowering function calls with the
rv_marker attribute. The goal is to expand such calls to the
following sequence of instructions:

BL @fn
mov x29, x29

This sequence of instructions triggers Objective-C runtime optimizations,
hence we want to ensure no instructions get moved in between them.
This patch achieves that by adding a new CALL_RVMARKER ISD node,
which gets turned into the BLR_RVMARKER pseudo, which eventually gets
expanded into the sequence mentioned above. The sequence is then marked
as instruction bundle, to avoid anything being moved in between.

@ahatanak is working on using this attribute in the front- & middle-end.

Together with the front- & middle-end changes, this should address
PR31925 for AArch64.

Diff Detail

Event Timeline

fhahn created this revision.Dec 3 2020, 4:19 AM
fhahn requested review of this revision.Dec 3 2020, 4:19 AM

Together with the front- & middle-end changes, this should address PR31925 for AArch64.

This one: https://bugs.llvm.org/show_bug.cgi?id=31925 ? I was looking for more context but didn't find it.

fhahn added a comment.Dec 4 2020, 7:55 AM

Together with the front- & middle-end changes, this should address PR31925 for AArch64.

This one: https://bugs.llvm.org/show_bug.cgi?id=31925 ? I was looking for more context but didn't find it.

Yes it's the right bug, but unfortunately does not have too much helpful information. Some more details about this optimization can be found here: https://opensource.apple.com/source/objc4/objc4-493.9/runtime/objc-arr.mm (search for Fast handling of returned autoreleased values).

I'll try to summarize the optimization. The goal is to reduce the overhead of objc_autoreleaseReturnValue in Callee followed by objc_retainAutoreleasedReturnValue in Caller in the snippet below. If they are back-to-back, then the ret object does not have to be added to the autorelease pool and which is much quicker.

To detect that pattern, objc_autoreleaseReturnValue looks at the instruction after ret = callee() in Caller. If it is the special marker instruction (mov x29, x29 on AArch64), then it assumes that it can do the optimization. The compiler is responsible for inserting this special marker. Currently it injects the marker as inline assembly (https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp#L483), but optimizations can move things in between he call and the marker. This patch ensures that nothing can move instructions between the call and the marker in the backend. @ahatanak will soon post a patch for the front- and middle-end.

Caller:
  ret = callee();
  objc_retainAutoreleasedReturnValue(ret);
  // use ret here

Callee:
  // compute ret
  [ret retain];
  return objc_autoreleaseReturnValue(ret);

As discussed offline, this also needs an implementation for GlobalISel. You can just add check lines to the test with -global-isel -global-isel-abort=1 for the tests.

fhahn updated this revision to Diff 309921.Dec 7 2020, 8:01 AM

Also move callsite info from BLR_RVMARKER to the expanded call. Add a MIR test case for BLR_RVMARKER expansion.

fhahn updated this revision to Diff 309961.Dec 7 2020, 11:07 AM

Handle expanding calls with multiple arguments.

fhahn updated this revision to Diff 309972.Dec 7 2020, 11:34 AM

As discussed offline, this also needs an implementation for GlobalISel. You can just add check lines to the test with -global-isel -global-isel-abort=1 for the tests.

Yep, but I think that can be done as a separate patch. The rv_attribute is not generated for -O0 for now, but I will add GlobalISel support as soon as SelDAG support is wrapped up. I added a -global-isel RUN line.

fhahn added a comment.Dec 8 2020, 12:07 PM

The patch that adds the attribute is available now as well: D92808

t.p.northover accepted this revision.Dec 11 2020, 6:23 AM

I think this looks reasonable now.

This revision is now accepted and ready to land.Dec 11 2020, 6:23 AM
This revision was landed with ongoing or failed builds.Dec 11 2020, 11:46 AM
This revision was automatically updated to reflect the committed changes.