<div dir="ltr">Revert and iterate sounds fine to me unless it is a small quick fix.<div><br></div><div>David</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, May 17, 2017 at 10:56 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">If there were issues raised in review and not addressed, it should probably be reverted until they're addressed.</div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote"><div dir="ltr">On Wed, May 17, 2017 at 11:52 AM Matthias Braun via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Dehao,<br>
<br>
can you please address these issues?<br>
<br>
I think this should at least:<br>
- Not consider PHIs as block-local definitions and move things towards them<br>
- Be conservative and stay away from cases involving different register classes so we do not constrain regalloc<br>
- Could this be moved to TargetPassConfig::<wbr>addPreRegAlloc() or similar so that targets can override and opt-out of using this pass?<br>
<br>
- Matthias<br>
<br>
> On May 17, 2017, at 10:40 AM, via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
> This patch caused some fairly significant regressions on our GPU target. While I can’t guarantee this is all of the problems, here is a particular case that exemplified to me “things this pass is doing that it should not”.<br>
><br>
><br>
> Original code:<br>
><br>
> PHI<br>
> PHI<br>
> PHI<br>
> […]<br>
> foo = REG_SEQUENCE […] // inputs come from the phi<br>
> STORE foo<br>
> bar = REG_SEQUENCE […]<br>
> STORE bar<br>
> foobar = REG_SEQUENCE […]<br>
> STORE foobar<br>
> […]<br>
><br>
> New code:<br>
><br>
> PHI<br>
> PHI<br>
> PHI<br>
> […]<br>
> foo = REG_SEQUENCE […]<br>
> bar = REG_SEQUENCE […]<br>
> foobar = REG_SEQUENCE […]<br>
> […]<br>
> STORE bar<br>
> STORE foo<br>
> STORE foobar<br>
><br>
> This causes a vast increase in register count because the reg_sequences constrain the registers to be adjacent far longer than necessary.<br>
><br>
> Two things are going on here that seem dubious.<br>
><br>
> 1. why are we moving them up… to a PHI??? does this make sense for any instruction?<br>
> 2. why are we moving reg_sequences??? away from their use, at that?<br>
><br>
> I can’t imagine this makes sense on any target.<br>
><br>
> —escha<br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div>
</div></div></blockquote></div><br></div>