No real comment on the issue itself but on the example as a former Eigen maintainer (sorry for the noise if that's all obvious for you):

Thanks, I've added a mir test that checks for fneg and gave it the name Matt suggested.

LGTM!

In D85649#2233902, @chill wrote:

In D85649#2231236, @tellenbach wrote:

Does this conflict with D80791?

It does, we're using different module attributes.

Does this conflict with D80791? Other than that it lgtm.

Hi @chill, thanks for this patch, looks like the correct way to handle this.

In D68076#2131108, @hiraditya wrote:

do we have a plan to follow up on this patch?

LGTM, thanks.

LGTM, but let's wait a bit for other comments.

Hi, thanks for this. The test case look very verbose and can probably be reduced. At least all unnecessary meta-information should be removed, e.g. does it really need all these function attributes?

In D71027#1862192, @jinlin wrote:

Is there any update for D69446? I am writing the test case for D71217. The changes D71027 have to be checked in first. Thanks.

Hi! Does this cover pre v8.3 pointer authenticating instructions (AUTIASP/AUTIBSP) too?

+1 to all comments above.

hasIllegalSPModification now only accepts matching sp modifying add and sub instructions of if they just increment/decrement sp.

Reopening this since it's still not correct.

In D71027#1775515, @jinlin wrote:

Sure. I have updated the patch. Please note that the repeating machine outliner may not work correctly without fixing the logic at line 1265.

As @lebedev.ri already said, is it possible to add a test for this? I know it is probably covered in your test for D71027 but an independent test would be great!

Reverse ping: Is this revision still active?

Okay, thanks for clarifying.

D69446 seems to introduce something similar. Are these patches related or is this just coincidence?

Thanks for correcting this comment. For possible future patches make sure to provide more context on your diff, i.e., run git diff -U99999. See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.

In D70635#1769906, @saugustine wrote:

In D70635#1769892, @tellenbach wrote:

@saugustine Thanks, I was just about to fix it with this patch but running the asan tests locally took longer than expected.

Do I need new review for this?

I wouldn't think so. It's been reviewed twice now and will be a useful change once it sticks. So conisder me accepting it.

@saugustine Thanks, I was just about to fix it with this patch but running the asan tests locally took longer than expected.

Thanks for the review!

this looks good beside some minor inline comments.

I've opened the following new revision that includes a fix for the bug in this patch and added @ostannard and @paquette again as reviewers: https://reviews.llvm.org/D70635

Add newline at eof

Address review comments.

Ping.

@ostannard The CFI instruction that can be omitted using this patch is the reason for D69097 (Return address signing for outlined functions) to be buggy, hence I've added you as a reviewer.

@ostannard wow, good catch! Thanks for reverting this.

LGTM, thanks!

I'd actually prefer an option outliner-runs that is defaulted to 1 rather than a rerun option. From a user perspective I'd be unclear what outliner-reruns=0 exactly means? Does that mean one run or zero?

Thanks for the great review!

• Disable outlining for functions that disagree on their v8.3A feature support
• Add a test for the above bullet point

• Improve support for v8.3a signing instructions
• Add tests
• Address review comments
• Moved signing logic into separate function

In D69482#1723832, @plotfi wrote:

In D69482#1723825, @tellenbach wrote:

Maybe I just don't see it but I don't get the reason for creating this new function doOutline. Doesn't it basically just call outline? Why can't you just add OutlinedFunctionNum to outline and then call outline in runOnModule?

Please be so kind to clarify this.

It’s possible to get greater size reduction through rerunning the outliner repeatedly. I and a couple other folks were looking into this and I noticed that much of the change could be refactored into an nfc change which is preferable.

Maybe I just don't see it but I don't get the reason for creating this new function doOutline. Doesn't it basically just call outline? Why can't you just add OutlinedFunctionNum to outline and then call outline in runOnModule?

Thanks for addressing the comments, LGTM!

Thanks for addressing the comments, LGTM.