[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:08:26 PDT 2016


MatzeB added a comment.

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


https://reviews.llvm.org/D22425





More information about the llvm-commits mailing list