Page MenuHomePhabricator

[MachineCopyPropagation] Rework how we manage RegMask clobbers
ClosedPublic

Authored by bogner on Sep 21 2018, 11:01 AM.

Details

Summary

Instead of updating the CopyTracker's maps each time we come across a
RegMask, defer checking for this kind of interference until we're
actually trying to propagate a copy. This avoids the need to
repeatedly iterate over maps in the cases where we don't end up doing
any work.

This is a slight compile time improvement for MachineCopyPropagation
as is, but it also enables a much bigger improvement that I'll follow
up with soon.

Diff Detail

Repository
rL LLVM

Event Timeline

bogner created this revision.Sep 21 2018, 11:01 AM
MatzeB accepted this revision.Sep 21 2018, 5:13 PM

LGTM

lib/CodeGen/MachineCopyPropagation.cpp
263 ↗(On Diff #166516)

Can you add a comment here along the lines of "we haven't checked regmasks yet, do this now".

Can this be expressed as range based for? (something like `for (const MachineInstr &MI : make_range(PrevCopy->getIterator(), Copy.getIterator()))`?

266–273 ↗(On Diff #166516)

why not just check the regmask operand inside the loop? Should make the code simpler and get away with the artifical restriction to a single regmask operand per instruction (not that there is any sensible usecase for multiple but anyway...)

This revision is now accepted and ready to land.Sep 21 2018, 5:13 PM
bogner updated this revision to Diff 166772.Sep 24 2018, 3:38 PM

I'd accidentally sent an old version of the patch for the review originally, which didn't quite work. This one is correct and also incorporates Matthias's feedback.

bogner marked 2 inline comments as done.Sep 24 2018, 3:40 PM
bogner added inline comments.
lib/CodeGen/MachineCopyPropagation.cpp
263 ↗(On Diff #166516)

Done and done. I'm not entirely convinced the range-for is any more readable in this case, but it isn't worse either.

575–577 ↗(On Diff #166772)

This was missing in the version of the patch I uploaded before, which led to use-after-frees.

MatzeB accepted this revision.Sep 24 2018, 4:07 PM

I'm convinced now the clobbering works correctly because as clobberRegister does not just clobber the register at hand but the whole destination register of the copy touched by the COPY at hand.
LGTM

bogner marked an inline comment as done.Sep 24 2018, 5:03 PM

I'm convinced now the clobbering works correctly because as clobberRegister does not just clobber the register at hand but the whole destination register of the copy touched by the COPY at hand.
LGTM

Was this meant for D52374? You already LGTM'd this one ;)

This revision was automatically updated to reflect the committed changes.