[PATCH] D110848: [TwoAddressInstruction] Fix ReplacedAllUntiedUses in processTiedPairs

Bjorn Pettersson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 4 13:02:51 PDT 2021


bjope added a comment.

In D110848#3040584 <https://reviews.llvm.org/D110848#3040584>, @MatzeB wrote:

>> 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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110848/new/

https://reviews.llvm.org/D110848



More information about the llvm-commits mailing list