[PATCH] D125755: [AggressiveInstcombine] Conditionally fold saturated fptosi to llvm.fptosi.sat

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

> The primary downside of target-specific transforms is that it goes against canonicalization: if a combine is expecting a specific form of IR, that form will only show up on specific targets.  So we either miss some transforms on some targets, or we write code to match the same thing in each possible form.
>
> It's always been a bit of a spectrum; transforms like inlining are fundamentally driven by heuristics, and those heuristics are going to lead to different IR on different targets.  But we want to encourage using canonical forms, even when we sometimes end up transforming from A->B, then later end up transforming B->A in the target's code generator.  This shapes the way we define IR to some extent; for example, llvm.cttz has an "is_zero_poison" flag so we can use the same intrinsic on all targets.
>
> This isn't to say we can never make a target-specific decision early, but we should explore alternatives that allow making a target-specific decisions later, where possible.

OK, I would like to get this and D113291 <https://reviews.llvm.org/D113291> unstuck if we can. There are other similar problems that would fall into the same camp. I agree that canonicalization can be very useful (and we should be careful not to break it needlessly), but that canonicalizations doesn't need to be identical for every target. It is always going to be detrimental if it is - if the costs being too incorrect to make correct decisions, or extra optimizations that could occur in the mid-end do not happen where they should. A single (semi-)target-independant canonicalization doesn't seem like a strong enough benefit to sacrifice optimization power or compile time.

For this patch - the transform needs to happen before loop unrolling to get the costs correct. And I feel is less about canonicalization as the transform is not reversible, neither form is canonical. There are other cost decisions like inlining but I believe they would be less likely to be super important. Which sounds like AggressiveInstCombine would be a good place for it, given its position in the pipeline. It is early enough to get the following costs correct, but doesn't muddy up instcombine with target queries.


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

https://reviews.llvm.org/D125755