[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