[PATCH] D134893: [LSR][TTI][RISCV] Add isAllowTerminatingConditionFoldingAfterLSR into TTI and enable it for RISC-V

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Ok, at this point all the known soundness problems are fixed in tree.  I was simply fixing ones obvious on inspection, and have not done any testing of this mechanism beyond the LIT tests themselves.

Before we move to discussing whether lsr-term-fold makes sense to enable by default, and on what basis, I think we need to have a discussion about validation and testing.  Whoever is going to drive this patch forward needs to describe their testing, and validate that after all the bug fixes this still triggers enough to be worthwhile.

I will note that I am generally skeptical of this being enabled on a per-target basis.  There needs to be a compelling argument as why this shouldn't be enabled more broadly.  One of the major advantages of enabling this globally is increasing testing, and thus smoking out bugs more quickly.  Given existing problems with testing, I think that advantage is one I'm very reluctant to give up.  I also think that all targets should benefit from the transform as current framed, so I don't see any reason not to enable it.  (I'm open to counter arguments here; they just need to be made.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134893