Page MenuHomePhabricator

[MIScheduler] Slightly better handling of constrainLocalCopy when both source and dest are local
ClosedPublic

Authored by mkuper on Jan 1 2015, 6:25 AM.

Details

Summary

This fixes PR21792 by handling an additional case in constrainLocalCopy.

This should probably be generalized, but I don't understand the code well enough to figure out how, yet.
Any suggestions on how to improve this are welcome.

Diff Detail

Repository
rL LLVM

Event Timeline

mkuper updated this revision to Diff 17754.Jan 1 2015, 6:25 AM
mkuper retitled this revision from to [MIScheduler] Slightly better handling of constrainLocalCopy when both source and dest are local.
mkuper updated this object.
mkuper edited the test plan for this revision. (Show Details)
mkuper added reviewers: atrick, qcolombet.
mkuper added a subscriber: Unknown Object (MLST).
qcolombet edited edge metadata.Jan 5 2015, 11:15 AM

Hi Michael,

I am not familiar with that code either but the current patch makes sense to me.
I leave the LGTM to Andy though.

Now looking for a generalization, I guess we could loop over all the local candidate and try to apply the transformation until we looked for all or one work.
I.e.,
Currently, we do:
pick a candidate
try apply

Your patch does:
pick a better candidate
try apply

The generalization would be:
for all candidate:

try apply
if success -> exit

Where the list of candidates is the local virtual registers.

Anyhow, Andy would have a much more educated answer.

Thanks,
-Quentin

Andy, ping?

atrick requested changes to this revision.Jan 15 2015, 11:18 AM
atrick edited edge metadata.

Thanks for the improvement!

Your test case comment lists an incorrect bug number: PR21972.

I'm not sure the logic makes sense to me. What if DstReg is local, and SrcReg is global, but a single interval? Now we'll just fail.

I don't think your example has any live intervals with "holes". I think you just wanted to change the bias from Local=DstReg to Local=SrcReg because the contraints will happen to work in that case.

I would avoid checking LI->size() == 1. I'm not sure it's really relevant. For example, two-address instructions may tie two segments together like this:
128B %vreg5<def,tied1> = SAR64ri %vreg5<tied0>, 32, %EFLAGS<imp-def,dead>; GR64_NOSP:%vreg5

I think you can just reverse the default global LI. It really looks like that's what I intended. I have no idea why I defaulted to SrcReg=Global. It may have just been messed up during code cleanup. I would try this:

+ If both the copy's source and dest are local live intervals, then we
+
should treat the dest as the global for the purpose of adding
+ // constraints. This adds edges from source's other uses to the copy.
+ unsigned LocalReg = SrcReg;
+ unsigned GlobalReg = DstReg;

If you do that, it looks like a trivial fix is needed in the widen_load-2.ll test.

This revision now requires changes to proceed.Jan 15 2015, 11:18 AM

Thanks a lot, Andrew!

As I wrote on the PR, I don't fully understand the surrounding code.
One of the constraints is that the "global" LI has at least one segment after the one that contains the start of the "local" LI. So a "global" LI with exactly one segment will never satisfy it, right? That's the case that I was trying to kill.

Anyway, if you're sure that treating the dest as the "global" works in all cases, I'm perfectly ok with that as well. :-)

Michael

mkuper updated this revision to Diff 18357.Jan 18 2015, 3:17 AM
mkuper edited edge metadata.

Changed according to Andrew's suggestion.

atrick accepted this revision.Jan 18 2015, 12:04 PM
atrick edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jan 18 2015, 12:04 PM
This revision was automatically updated to reflect the committed changes.