This is an archive of the discontinued LLVM Phabricator instance.

[TwoAddressInstruction] Fix ReplacedAllUntiedUses in processTiedPairs
ClosedPublic

Authored by foad on Sep 30 2021, 9:39 AM.

Details

Summary

Fix the calculation of ReplacedAllUntiedUses when any of the tied defs
are early-clobber. The effect of this is to fix the placement of kill
flags on an instruction like this (from @f2 in
test/CodeGen/SystemZ/asm-18.ll):

INLINEASM &"stepb $1, $2" [attdialect], $0:[regdef-ec:GRH32Bit], def early-clobber %3:grh32bit, $1:[reguse tiedto:$0], killed %4:grh32bit(tied-def 3), $2:[reguse:GRH32Bit], %4:grh32bit

After TwoAddressInstruction without this patch:

%3:grh32bit = COPY killed %4:grh32bit
INLINEASM &"stepb $1, $2" [attdialect], $0:[regdef-ec:GRH32Bit], def early-clobber %3:grh32bit, $1:[reguse tiedto:$0], %3:grh32bit(tied-def 3), $2:[reguse:GRH32Bit], %4:grh32bit

Note that the COPY kills %4, even though there is a later use of %4 in
the INLINEASM. This fails machine verification if you force it to run
after TwoAddressInstruction (currently it is disabled for other
reasons).

After TwoAddressInstruction with this patch:

%3:grh32bit = COPY %4:grh32bit
INLINEASM &"stepb $1, $2" [attdialect], $0:[regdef-ec:GRH32Bit], def early-clobber %3:grh32bit, $1:[reguse tiedto:$0], %3:grh32bit(tied-def 3), $2:[reguse:GRH32Bit], %4:grh32bit

Event Timeline

foad created this revision.Sep 30 2021, 9:39 AM
foad requested review of this revision.Sep 30 2021, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2021, 9:39 AM
MatzeB added a comment.Oct 1 2021, 9:44 AM

Could you add a .mir test?

foad updated this revision to Diff 376589.Oct 1 2021, 11:44 AM

Rebase on a test case that I will pre-commit.

Have you considered .mir tests instead of .ll tests? They should be more robust as they allow you to test a single pass independently of anything happening before/afterwards. See D36224 or test/CodeGen/PowerPC/two-address-crash.mir for examples. There is also llvm/utils/update_mir_test_checks.py to autogenerate the CHECK lines.

foad added a comment.Oct 4 2021, 11:36 AM

Have you considered .mir tests instead of .ll tests? They should be more robust as they allow you to test a single pass independently of anything happening before/afterwards.

Yes I considered it, but I think there are advantages to both approaches. The problem with a MIR test case is that if the target changes its instruction definitions in any way, updating the MIR input is definitely a manual process.

That said, I'm happy to do a MIR test instead if you still prefer it.

Yes I considered it, but I think there are advantages to both approaches. The problem with a MIR test case is that if the target changes its instruction definitions in any way, updating the MIR input is definitely a manual process.

That said, I'm happy to do a MIR test instead if you still prefer it.

The other side of the medal is that the test now depends on the all the passes before to behave the same, so changes to instruction selection, calling convention modeling etc. throw off the tests.

While I may be biased here as to what is more common, I think having a unit test as targetted as possible (1 pass only) is a good principle to aim for.

MatzeB accepted this revision.Oct 4 2021, 12:08 PM

LGTM

This revision is now accepted and ready to land.Oct 4 2021, 12:08 PM
bjope added a comment.Oct 4 2021, 1:02 PM

Yes I considered it, but I think there are advantages to both approaches. The problem with a MIR test case is that if the target changes its instruction definitions in any way, updating the MIR input is definitely a manual process.

That said, I'm happy to do a MIR test instead if you still prefer it.

The other side of the medal is that the test now depends on the all the passes before to behave the same, so changes to instruction selection, calling convention modeling etc. throw off the tests.

While I may be biased here as to what is more common, I think having a unit test as targetted as possible (1 pass only) is a good principle to aim for.

In this case there aren't that many target instructions. All I see in the CHECK:s is COPY and INLINEASM. And maybe it can be reduced even further if using a MIR input?
(well, I won't stop this as is, just thinking that the reasons for not using MIR in this particular case seemed pretty weak)

Btw, the fix looks fine!

foad updated this revision to Diff 377763.Oct 7 2021, 1:54 AM

Switch to MIR test case.

This revision was landed with ongoing or failed builds.Oct 7 2021, 2:10 AM
This revision was automatically updated to reflect the committed changes.
bjope added a comment.Oct 7 2021, 2:11 AM

Thank! LGTM!

foad added a comment.Oct 7 2021, 2:11 AM

(well, I won't stop this as is, just thinking that the reasons for not using MIR in this particular case seemed pretty weak)

Agreed!

Btw, the fix looks fine!

Thanks!