[PATCH] D66029: llvm-canon

Tue Aug 13 04:57:49 PDT 2019

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?

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