Page MenuHomePhabricator

[PostRASink]Add register dependency check for implicit operands
ClosedPublic

Authored by junbuml on Mar 27 2018, 2:41 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

junbuml created this revision.Mar 27 2018, 2:41 PM
junbuml updated this revision to Diff 140009.Mar 27 2018, 2:47 PM

Sorry for the delay.

Thanks for fixing this. What do you think about using register units throughout the pass? I found them so much clearer to use when dealing with register aliases. @MatzeB has a great presentation on it: 2016 LLVM Developers' Meeting: M. Braun "Dealing with Register Hierarchies".

lib/CodeGen/MachineSink.cpp
1080 ↗(On Diff #140009)

Would MO.readsReg() be a better fit here?

1127 ↗(On Diff #140009)

I would comment on what the unsigned values represent.

What do you think about using register units throughout the pass? I found them so much clearer to use when dealing with register aliases.

I think I saw the case where LiveRegUnits is used to find a scratch reg. It seems possible to use it here as well. Let me take a little closer look at it. I think it should be a separate NFC patch.

lib/CodeGen/MachineSink.cpp
1080 ↗(On Diff #140009)

One case where readReg works better is when the source of Copy is undef, but I'm not perfectly clear if it's safe to skip the internal read.

junbuml updated this revision to Diff 141676.Apr 9 2018, 10:07 AM

Just minor update in comments.

junbuml added inline comments.Apr 9 2018, 10:10 AM
lib/CodeGen/MachineSink.cpp
1091 ↗(On Diff #141676)

I keep isUse() here with FIXME since I'm not perfectly sure if skipping the internal read is safe in all other targets.

thegameg accepted this revision.Apr 12 2018, 4:00 AM

I think I saw the case where LiveRegUnits is used to find a scratch reg. It seems possible to use it here as well. Let me take a little closer look at it. I think it should be a separate NFC patch.

Even without LiveRegUnits, I think you could replace ModifiedRegs and UsedRegs to be ModifiedRegUnits and UsedRegUnits, and use MCRegUnitIterators to check if registers alias.

I'm ok to commit this and follow-up in separate patches for the rest. Thanks!

lib/CodeGen/MachineSink.cpp
1126 ↗(On Diff #140009)

Any particular reason why we need to store both the register and the operand idx?

1091 ↗(On Diff #141676)

Ok, sounds good to me.

This revision is now accepted and ready to land.Apr 12 2018, 4:00 AM

Even without LiveRegUnits, I think you could replace ModifiedRegs and UsedRegs to be ModifiedRegUnits and UsedRegUnits, and use MCRegUnitIterators to check if registers alias.
I'm ok to commit this and follow-up in separate patches for the rest. Thanks!

Make sense to me. I will do this in a separate patch.

lib/CodeGen/MachineSink.cpp
1126 ↗(On Diff #140009)

The operand idx will be used in clearKillFlags() to set the kill flag in src reg of the Copy sunk if the src reg is killed in an instruction placed between Copy and the end of the block.

thegameg added inline comments.Apr 12 2018, 11:02 AM
lib/CodeGen/MachineSink.cpp
1126 ↗(On Diff #140009)

Right, I was just curious why not only use the operand idx and use MI.getOperand(OpIdx).getReg() to retrieve the register.

junbuml added inline comments.Apr 12 2018, 11:23 AM
lib/CodeGen/MachineSink.cpp
1126 ↗(On Diff #140009)

I see what you mean. I will change it to keep only operand idx and get the register using MI.getOperand(OpIdx).getReg() .

junbuml updated this revision to Diff 142259.Apr 12 2018, 2:15 PM

Addressed Francis' comment.

LGTM, thanks!

This revision was automatically updated to reflect the committed changes.