This is an archive of the discontinued LLVM Phabricator instance.

Use TRI->regsOverlap() in MachineBasicBlock::computeRegisterLiveness
ClosedPublic

Authored by uabelho on Sep 24 2018, 5:17 AM.

Details

Summary

For the loop that used MCRegAliasIterator this should be NFC.

For the loop that previously used MCSubRegIterator we should
now detect more cases where the register is actually live out that
we previously missed.

Diff Detail

Repository
rL LLVM

Event Timeline

uabelho created this revision.Sep 24 2018, 5:17 AM

I would like to create a testcase exposing that we actually fix a bug when we use regsOverlap() instead of MCSubRegIterator, but then I think I need help.

In https://reviews.llvm.org/D51348 Matt suggested it can be triggered in an "AMDGPU testcase where VCC_LO or VCC_HI is defined and VCC is the live out".
I'm not sure about this though. I don't know AMDGPU or how computeRegisterLivenessis used there, but just by looking at the definition of VCC I'm not sure
it's enough to trigger the problem:

def VCC : RegisterWithSubRegs<"vcc", [VCC_LO, VCC_HI]>,

DwarfRegAlias<VCC_LO> {

Since both VCC_LO and VCC_HI are sub registers of VCC I think the old MCSubRegIterator code works?

What we would need would be some register VCC_X that is made up of both VCC_LO or VCC_HI _and_ some other part so it overlaps/aliases VCC, but it's not
a sub register of VCC.

An instruction just needs to def one or the other and the live out would be vcc, since it needs to visit the super register

An instruction just needs to def one or the other and the live out would be vcc, since it needs to visit the super register

Hm, you think any of the cases in test/CodeGen/AMDGPU/fold-immediate-operand-shrink.mir could be copied and modifed
into exposing it then? Any idea which one would be most appropriate?

MatzeB accepted this revision.Sep 24 2018, 5:33 PM

code change LGTM.
I'd be great if you can find a test for this. (Just in case the AMDGPU based test doesn't work out I'd be fine without one here, given how rarely computeRegisterLiveness() is used)

This revision is now accepted and ready to land.Sep 24 2018, 5:33 PM

An instruction just needs to def one or the other and the live out would be vcc, since it needs to visit the super register

Hm, you think any of the cases in test/CodeGen/AMDGPU/fold-immediate-operand-shrink.mir could be copied and modifed
into exposing it then? Any idea which one would be most appropriate?

It doesn't actually work. The query is on the passed register, which is always the super register so this doesn't apply

Alright, I'll commit this as is in a little bit then. Thanks!

This revision was automatically updated to reflect the committed changes.