[PATCH] D22425: ExpandPostRAPseudos should transfer implicit uses, not only implicit defs

Matthias Braun via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 15 15:11:31 PDT 2016


MatzeB added a comment.

In https://reviews.llvm.org/D22425#486017, @MatzeB wrote:

> In https://reviews.llvm.org/D22425#486010, @mkuper wrote:
>
> > In https://reviews.llvm.org/D22425#485991, @MatzeB wrote:
> >
> > > If BL is defined, it does not mean that the other lanes are dead before the instruction. So there is still a bug in the anti dependency breaker.
> >
> >
> > If I understand the code there correctly, the anti dependency breaker will not declare the super-register dead just because the sub-register is dead, it actually needs a def (or imp-def) of the super-register. So I'm not sure there's still a bug.
> >
> > > On the other hand this patch is obviously good and there is no reason not to do it, so LGTM.
> >
> > >  Have you considered writing a .mir test ("llc -run-pass postrapseudos ...")? That should give you a much more stable test.
> >
> >
> > Is MIR is stable enough to write tests *in* it, not just *for* it?
> >  I'd go for that if I were unable to come up with an IR test, but since the IR test looks fairly sane, I'd prefer to keep it.
>
>
> MIR still has clunky syntax and if you are unlucky crashs, but it is certainly good enough for most situations nowadays.
>
>   > ls PowerPC/*.mir X86/*.mir AArch64/*.mir | wc -l
>    14
>   
>    It worked for 14 real world tests already.
>   
>
> For cases like this I really like the fact that the test is not depend on isel and other unrelated passes which IMO even outweights the clunky syntax...
>
> - Matthias


That said I'm fine with keeping the .ll test here.


https://reviews.llvm.org/D22425





More information about the llvm-commits mailing list