[PATCH] D32563: Add LiveRangeShrink pass to shrink live range within BB.

[Dehao Chen via Phabricator via llvm-commits]

danielcdh marked an inline comment as done.
danielcdh added a comment.

In https://reviews.llvm.org/D32563#747436, @andreadb wrote:

> In https://reviews.llvm.org/D32563#747423, @danielcdh wrote:
>
> > In https://reviews.llvm.org/D32563#747393, @andreadb wrote:
> >
> > >
> >
> >
> > Sorry, did not address this in the last patch. Done now. Note that I do not want to touch unnecessary files in this change. So I simply copied the code from MachineSink.cpp, and will refactor it to a utility function in MachineInstr.h/.cpp in a follow-up patch.
>
>
> No problem. Thanks!
>
> >> About your new test case. I think a MIR test would be even better. That said, I don't have a strong opinion (I am okay with your new test).
> >>  In case, could you please regenerate the CHECK lines using `update_llc_test_checks.py? The test should be small, so I don't expect too many CHECK lines inserted by the script.
> > 
> > Done, but I'm not sure if that's the right thing to do. update_llc_test_checks makes the test too fragile: any subtle change of the codegen would break it. And it hides what the author really want to test and follow-up patch writers may simply run the script and regenerate the test (without understanding what it's testing, or they need to spend much time on what the original purpose is). BTW, what's MIR test? could you point me to one example? Thanks.
>
> Right, I think it your test is okay.
>
> About MIR tests, you can have a look at this page: http://llvm.org/docs/MIRLangRef.html
>  In particular, section "MIR Testing Guide" (and the next two sub-sections) are quite useful imho.
>
> For example, you could do something like:
>
> > llc my-new-test.ll -stop-after=machine-sink -o test.mir
>
> Then, test.mir with a RUN line which only tests your new pass
>  For example:
>
> > llc -o - %s -mtriple=x86_64-- -run-pass=lrshrink | FileCheck %s
>
> The advantage is that you test your new pass in isolation.
>  But, as I wrote, I think your test is okay.


Thanks for the explanation. I've reverted the test to its original form. And after checking the mir test output, I think it's better to keep it simple in its original phone, and make sure that later scheduler does not undo the change in this patch.



================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:112
+      ++Next;
+      if (MI->isPHI() || MI->isCompare() || MI->isDebugValue())
+        continue;
----------------
gberry wrote:
> danielcdh wrote:
> > gberry wrote:
> > > Why are you skipping compares?
> > In many architectures, cmp should stay together with jmp instruction for performance purpose. We do not want to break this in this pass.
> For targets that don't have allocatable flag regsiters, your later check for physical regs should prevent moving these compares (and any other non-compare flag setting instructions).  For targets that keep the results of compares in general purpose registers, this seems like a missed optimization.  Also, if it is important that the compare be just before the branch, the scheduler should be taking care of that.
> 
> If you don't agree that this check should be removed can you put a comment explaining why this check for compares is being done?
That makes sense. I've removed the "compare" check.


https://reviews.llvm.org/D32563