[PATCH] D66029: llvm-canon

[Puyan Lotfi via llvm-commits]

On Tue, Aug 13, 2019 at 4:57 AM Michal Paszkowski via Phabricator via
llvm-commits <llvm-commits at lists.llvm.org> wrote:

> mpaszkowski added a comment.
>
> First of all, thank you for your valuable feedback!
>
> In D66029#1623654 <https://reviews.llvm.org/D66029#1623654>, @lebedev.ri
> wrote:
>
> > Please run clang-format on the patch.
> >  What is the plan for tests here?
>
>
> Sure, will run clang-format!
>
> 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.
> Can you think of any way how to test it sensibly?
>
>

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:

https://github.com/llvm/llvm-project/commit/14b6637edc41125997bf395f1a3504e7288b0e93

```
    ;CHECK: %namedVReg1352:gpr32 = LDRWui %stack.0, 0 :: (dereferenceable
load 8)
    ;CHECK: $w0 = COPY %namedVReg1352
    ;CHECK: RET_ReallyLR implicit $w0

    %vreg1234:gpr32 = COPY %42
    %vreg1235:gpr32 = COPY %vreg1234
    %vreg1236:gpr32 = COPY %vreg1235
    $w0 = COPY %vreg1236
    RET_ReallyLR implicit $w0
```

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.

I'm also really interested in seeing the IR Canonicalization pass moved
into llvm/lib instead of llvm/tools/llvm-canon.


PL



> In D66029#1623760 <https://reviews.llvm.org/D66029#1623760>, @hfinkel
> wrote:
>
> > 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.
> >
> > 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.
>
>
> I will take a look at PHI nodes and move the pass to Transforms.
>
> In D66029#1626100 <https://reviews.llvm.org/D66029#1626100>,
> @alexandre.isoard wrote:
>
> > Seems like a great idea!
> >
> > 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.
> >  Also, in my personal implementation I had missed //anonymous types//
> and //anonymous global variable//, I don't know if we captured those here.
> >
> > Note that I'm not sure if we can name all metadata or function attribute
> lists.
>
>
> 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.
>
>
> Repository:
>   rL LLVM
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D66029/new/
>
> https://reviews.llvm.org/D66029
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190813/6f05bf8f/attachment.html>