Page MenuHomePhabricator

SimplifyCFG SinkCommonCodeFromPredecessors: Also sink function calls without used results (PR41259)
ClosedPublic

Authored by hans on Mar 28 2019, 7:56 AM.

Details

Summary

The code was previously checking that candidates for sinking had exactly one use or were a store instruction (which can't have uses). This meant we could sink call instructions only if they had a use.

That limitation seemed a bit arbitrary, so this patch changes it to "instruction has zero or one use" which seems more natural and removes the need to special-case stores.

Please take a look!

Diff Detail

Repository
rL LLVM

Event Timeline

hans created this revision.Mar 28 2019, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2019, 7:56 AM
hans added inline comments.Mar 28 2019, 8:22 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1471 ↗(On Diff #192635)

Actually this isn't quite right. I0 might not have a use but one of the other instructions might, or vice-versa. Same problem for the checks below...

hans updated this revision to Diff 192657.Mar 28 2019, 9:30 AM

Checking that the instructions have the same number of uses. This passes a clang bootstrap build.

hans marked an inline comment as done.Mar 28 2019, 9:31 AM

Maybe Ben wants to take a look?

dmgreen added a subscriber: dmgreen.Apr 1 2019, 5:53 AM
dmgreen accepted this revision.Apr 1 2019, 6:40 AM

Looks like a good improvement to me.

This revision is now accepted and ready to land.Apr 1 2019, 6:40 AM
hans added a comment.Apr 2 2019, 12:59 AM

Looks like a good improvement to me.

Thanks!

This revision was automatically updated to reflect the committed changes.

Hi,

When looking at some code changes for my out-of-tree target with this commit I noticed a change that I thought I'd ask if you think
is good or not.

With this change we now seem to sink e.g. calls to @llvm.lifetime.end? It returns void, and thus has 0 uses, and as far as I understand
such calls were not sinked before? Is this on purpose and is that good? In general I guess it's good to do lifetime.end at soon as possible?

(And I suppose there are also a whole bunch of other intrinsics that also return void that can be sinked now, I've no idea if that
can cause problems somewhere?)

hans added a comment.Apr 3 2019, 1:29 AM

When looking at some code changes for my out-of-tree target with this commit I noticed a change that I thought I'd ask if you think
is good or not.

With this change we now seem to sink e.g. calls to @llvm.lifetime.end? It returns void, and thus has 0 uses, and as far as I understand
such calls were not sinked before? Is this on purpose and is that good? In general I guess it's good to do lifetime.end at soon as possible?

(And I suppose there are also a whole bunch of other intrinsics that also return void that can be sinked now, I've no idea if that
can cause problems somewhere?)

It's not on purpose in the sense that I was targeting the lifetime intrinsics, but it's showing my change working as intended.

For the lifetime intrinsics, I don't think this really matters. As you say, it's good to do lifetime.end as soon as possible, so sinking them in general doesn't make sense, but I also don't think this matters much here because they can't be sunk very far, i.e. I don't see how this sinking could actually extend the lifetime.

When looking at some code changes for my out-of-tree target with this commit I noticed a change that I thought I'd ask if you think
is good or not.

With this change we now seem to sink e.g. calls to @llvm.lifetime.end? It returns void, and thus has 0 uses, and as far as I understand
such calls were not sinked before? Is this on purpose and is that good? In general I guess it's good to do lifetime.end at soon as possible?

(And I suppose there are also a whole bunch of other intrinsics that also return void that can be sinked now, I've no idea if that
can cause problems somewhere?)

It's not on purpose in the sense that I was targeting the lifetime intrinsics, but it's showing my change working as intended.

For the lifetime intrinsics, I don't think this really matters. As you say, it's good to do lifetime.end as soon as possible, so sinking them in general doesn't make sense, but I also don't think this matters much here because they can't be sunk very far, i.e. I don't see how this sinking could actually extend the lifetime.

Ok, yes I don't know, when I saw it it just worried me that perhaps intrinsics like lifetime.end was the "arbitrary" reason that we didn't allow to sink zero-use instructions before.

hans added a comment.Apr 3 2019, 2:19 AM

For the lifetime intrinsics, I don't think this really matters. As you say, it's good to do lifetime.end as soon as possible, so sinking them in general doesn't make sense, but I also don't think this matters much here because they can't be sunk very far, i.e. I don't see how this sinking could actually extend the lifetime.

Ok, yes I don't know, when I saw it it just worried me that perhaps intrinsics like lifetime.end was the "arbitrary" reason that we didn't allow to sink zero-use instructions before.

The way I imagine it happened was that when support for sinking was added, it carefully handled the case of uses, dealing with the PHI node and so on. Then someone realized we couldn't sink store instructions because they don't have uses, and added a special case for them.

We saw some good looking codesize decreases from this, by the way. Just to add to the "this patch does good" side of the argument :)

For lifetime.end, if we start producing llvm.lifetime.end(phi(x, y, z)) then I could see that potentially being worse. I have no evidence of that happening though. And it may even be better if it allows further sinking.

Maybe you can add more tests with these intrinsics?

dlj added a subscriber: dlj.Apr 3 2019, 7:27 PM

This revision causes tests to fail under ASAN. After some investigation (with Chandler's help), it looks like the safest course of action is to revert. We're following up with Hans separately.

Reverted as r357667.

hans added a comment.Apr 4 2019, 12:51 AM
In D59936#1454346, @dlj wrote:

This revision causes tests to fail under ASAN. After some investigation (with Chandler's help), it looks like the safest course of action is to revert. We're following up with Hans separately.

Reverted as r357667.

Thanks, and sorry for the breakage.

For my own notes, the internal bug tracker entry is 129905821. The V8 team also saw asan errors from ICU due to this, tracked in https://bugs.chromium.org/p/v8/issues/detail?id=9086 I'll try to investigate there to keep this in the open.

nick added a subscriber: nick.Apr 5 2019, 7:05 AM
hans added a comment.Apr 12 2019, 5:16 AM
In D59936#1454346, @dlj wrote:

This revision causes tests to fail under ASAN. After some investigation (with Chandler's help), it looks like the safest course of action is to revert. We're following up with Hans separately.

Reverted as r357667.

Thanks, and sorry for the breakage.

For my own notes, the internal bug tracker entry is 129905821. The V8 team also saw asan errors from ICU due to this, tracked in https://bugs.chromium.org/p/v8/issues/detail?id=9086 I'll try to investigate there to keep this in the open.

And https://bugs.llvm.org/show_bug.cgi?id=41481 for the asan issue.

This points to an interesting point that was raised here earlier though:

For lifetime.end, if we start producing llvm.lifetime.end(phi(x, y, z)) then I could see that potentially being worse. I have no evidence of that happening though. And it may even be better if it allows further sinking.

We now know that it did happen :-) In the case with the asan false positive it was two lifetime.start intrinsics sunk into one with a select argument.

But I think that is working as intended. It doesn't break any invariants with the intrinsics that I know of, and I don't think it will generate worse code either. In fact, I think seeing those lifetime intrinsics getting sunk shows that the intrinsics were blocking sinking optimizations that we are now able to perform.

What are you thinking is best here? I agree that lifetime.end shouldn't prohibit optimisations. Do you think it's best to fix asan and go with this as-is, or to try and prevent sinking if it will only sink a lifetime.time?

hans added a comment.Apr 16 2019, 1:02 AM

What are you thinking is best here? I agree that lifetime.end shouldn't prohibit optimisations. Do you think it's best to fix asan and go with this as-is, or to try and prevent sinking if it will only sink a lifetime.time?

I think it's best to fix asan and re-commit this as-is. I'll do that once the asan fix (r358478) has baked a little.

OK. Sounds good to me. We can always change it if necessary.

hans added a comment.Apr 16 2019, 5:11 AM

Re-committed in r358483.