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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

In D125755#3524383 <https://reviews.llvm.org/D125755#3524383>, @dmgreen wrote:

>> So I'm not opposed, but I also don't have a good sense about potential downside of allowing target-specific IR transforms earlier in the optimization pipeline. Clearly, there's demand to do this because we get this kind of request fairly regularly.
>
> Do we know what the downsides presented in the past were? I've heard about an increased need for testing on all targets before, but that has really always been true with datalayout controlled combines. I believe that was more about the core of instcombine though, where more fundamental canonicalizations are happening. This and D113291 <https://reviews.llvm.org/D113291> I feel are more about higher level irreversible transforms.

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.


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

https://reviews.llvm.org/D125755