<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Aug 13, 2019 at 4:57 AM Michal Paszkowski via Phabricator via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">mpaszkowski added a comment.<br>
<br>
First of all, thank you for your valuable feedback!<br>
<br>
In D66029#1623654 <<a href="https://reviews.llvm.org/D66029#1623654" rel="noreferrer" target="_blank">https://reviews.llvm.org/D66029#1623654</a>>, @lebedev.ri wrote:<br>
<br>
> Please run clang-format on the patch.<br>
>  What is the plan for tests here?<br>
<br>
<br>
Sure, will run clang-format!<br>
<br>
When it comes to tests, I don't have any plan yet. I am not sure if testing every scenario is the best solution here - the canonicalization techniques may change easily as these are my 'best' approaches.<br>
Can you think of any way how to test it sensibly?<br>
<br></blockquote><div><br></div><div><br></div><div>There is always a way to test. Even if your test is a little bit brittle that's still better than nothing. Ideally for every feature and technique of llvm-canon, you will have a lit test. For example, when I did copy folding, I wrote this (albeit very lame) test:</div><div><br></div><div><a href="https://github.com/llvm/llvm-project/commit/14b6637edc41125997bf395f1a3504e7288b0e93">https://github.com/llvm/llvm-project/commit/14b6637edc41125997bf395f1a3504e7288b0e93</a><br></div><div><br></div><div>```</div><div>    ;CHECK: %namedVReg1352:gpr32 = LDRWui %stack.0, 0 :: (dereferenceable load 8)<br>    ;CHECK: $w0 = COPY %namedVReg1352<br>    ;CHECK: RET_ReallyLR implicit $w0<br><br>    %vreg1234:gpr32 = COPY %42<br>    %vreg1235:gpr32 = COPY %vreg1234<br>    %vreg1236:gpr32 = COPY %vreg1235<br>    $w0 = COPY %vreg1236<br>    RET_ReallyLR implicit $w0<br></div><div>```</div><div><br></div><div>Even if its just a couple lit tests for a few happy path cases, for each canonicalization technique, that's still pretty a good thing. If the output changes in the future, update the tests to reflect that. </div><div><br></div><div>I'm also really interested in seeing the IR Canonicalization pass moved into llvm/lib instead of llvm/tools/llvm-canon.</div><div><br></div><div><br></div><div>PL</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
In D66029#1623760 <<a href="https://reviews.llvm.org/D66029#1623760" rel="noreferrer" target="_blank">https://reviews.llvm.org/D66029#1623760</a>>, @hfinkel wrote:<br>
<br>
> I also recommend that you canonicalize PHI nodes. In past experiments looking for fixed points in the optimization pipeline, this came up as a significant issue. The order of the predecessors in the PHI operand lists don't carry any significance, also also sometimes a predecessor can be listed multiple times (always with the same corresponding value). It's probably best to canonicalize those so each predecessor is listed only once and the blocks appear in their natural order.<br>
><br>
> At a high level, I'd much rather see the underlying logic under lib/Transforms/Utils, and then we can just run this with opt and we don't need a separate utility.<br>
<br>
<br>
I will take a look at PHI nodes and move the pass to Transforms.<br>
<br>
In D66029#1626100 <<a href="https://reviews.llvm.org/D66029#1626100" rel="noreferrer" target="_blank">https://reviews.llvm.org/D66029#1626100</a>>, @alexandre.isoard wrote:<br>
<br>
> Seems like a great idea!<br>
><br>
> Could we have an option to only **rename without reordering**? I have found, in the past, some issue that were order sensitive, but rarely name sensitive, and it would be great to be able to debug those.<br>
>  Also, in my personal implementation I had missed //anonymous types// and //anonymous global variable//, I don't know if we captured those here.<br>
><br>
> Note that I'm not sure if we can name all metadata or function attribute lists.<br>
<br>
<br>
I will add the option to run just naming or reordering - both stages are independent. We also haven't considered anonymous types and anonymous global variables.<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D66029/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D66029/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D66029" rel="noreferrer" target="_blank">https://reviews.llvm.org/D66029</a><br>
<br>
<br>
<br>
_______________________________________________<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="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>