Page MenuHomePhabricator

[X86]: Don't set a regmask on conditional tail calls (PR31257)
AbandonedPublic

Authored by hans on Feb 3 2017, 1:21 PM.

Details

Reviewers
mkuper
Summary

The regmask would cause analyses to think the conditional tail call clobbers registers. Specifically, Machine Copy Propagation would remove a copy instruction to %ecx before the tail call (see PR).

Since the tail call is conditional, registers are only clobbered if the condition is met, in which case control leaves the function so it doesn't matter. Therefore, model the instruction as not clobbering any registers.

Diff Detail

Event Timeline

hans created this revision.Feb 3 2017, 1:21 PM
mkuper edited edge metadata.Feb 6 2017, 11:50 AM

Sorry for the lag, Hans.

I thought about this a bit more, and I'm not sure I'm a fan of this, even though I basically suggested something similar on Friday.

Two issues:

  1. I'm still not sure this won't have any unintended consequences.
  2. Having BuildMI return an instruction that requires you to remove an operand for correctness seems rather inelegant. I agree that this is probably "safer" than removing IsCall, and that is probably the only code that will ever construct a TCRETURNdi64cc MI, but, still...

I don't have any good ideas, though.

hans added a comment.Feb 6 2017, 3:32 PM

Sorry for the lag, Hans.

I thought about this a bit more, and I'm not sure I'm a fan of this, even though I basically suggested something similar on Friday.

Two issues:

  1. I'm still not sure this won't have any unintended consequences.
  2. Having BuildMI return an instruction that requires you to remove an operand for correctness seems rather inelegant. I agree that this is probably "safer" than removing IsCall, and that is probably the only code that will ever construct a TCRETURNdi64cc MI, but, still...

    I don't have any good ideas, though.

I agree with 2), that code is clumsy at best. I think what happened is that we regressed this in r281223: we shouldn't have copied the regmask in the first place. We don't have to remove an operand for correctness, it was incorrect to add it. I'm uploading a new patch that does this.

As for constructing TCRETURNdi64cc elsewhere, well they shouldn't put regmask on it either.

For 1) well, that's the case with a lot of code. I'm feeling pretty good about this change though. (I don't think removing isCall would actually work, as the regmask is copied over from the unconditional tail call. I suppose we could remove isCall from that, but that doesn't feel right at all.)

hans updated this revision to Diff 87325.Feb 6 2017, 3:33 PM

Don't copy the regmask in teh first place.

mkuper added a comment.Feb 6 2017, 5:15 PM

What's special about this code (and why I'm reluctant for it to go in) is that we're "lying" :-) That is, we're pretending an instruction that may clobber registers doesn't. That kind of stuff tends to be brittle.
Also, it's inconsistent - if the reasoning is that a TCRETURN doesn't need to provide a regmask (because which registers a return "clobbers" doesn't matter), then I guess we should fix this by never attaching a mask to any TCRETURN. Then you also won't have the problem of copying it here, since the original TCRETURN won't have one either.

Re 2 - I didn't notice where the regmask was actually coming from! Yes, this makes much more sense. But see above.

hans abandoned this revision.Feb 7 2017, 1:15 PM

I've reverted the conditional tail calls in r294348 to fix trunk and the branch while we figure out the proper solution. I've CC'd Quentin on the bug so we can discuss it there.