[PATCH] D87551: [LICM] Make Loop ICM profile aware again

Thu Oct 14 14:23:31 PDT 2021

lebedev.ri added a comment.

In D87551#3065340 <https://reviews.llvm.org/D87551#3065340>, @reames wrote:

> I'm sorry for coming to this very late, but I just noticed this patch due to discussion on another patch (D111806 <https://reviews.llvm.org/D111806>).
>
> This change is inconsistent with the long standing design of LICM.  LICM is a canonicalizing transform, and we specifically *do not* use any profitability metric therein.  MachineLICM exists specifically to be the inverse transformation which is cost based.  I see nothing in the example to make it clear why MachineLICM could not handle this case.
>
> As evidenced by the very existence of the later patch, having this in tree sets a bad precedent.  I believe this patch should be reverted.
>
> Given it's been in tree for nearly a year, this clearly isn't immediately urgent.  Given that, I'm going to give a couple of days for discussion before reverting this patch.

FWIW, i agree with Philip on this.

While i'm sure that this, and many other, transform are easy wins,
they fundamentally do not fit the design of the pass(es). Creating initial divergence
opens a door for further changes of the same nature, and it's increasingly more
and more complicated to keep the door shut further down the line. The other example
is e.g. instcombine. We just can not afford to have a single canonical representation
except not really when $X || $Y || $z.

This must be rolled back.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87551/new/

https://reviews.llvm.org/D87551