[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In D81728#2089713 <https://reviews.llvm.org/D81728#2089713>, @spatel wrote:

> In D81728#2089644 <https://reviews.llvm.org/D81728#2089644>, @Flakebi wrote:
>
> > This combines instructions, so I think it belongs into the InstCombine pass. On the other hand, the f16 form of the intrinsics is not available on all targets, so this combination cannot be applied unconditionally but it needs to be gated depending on the target.
>
>
> The fact that this pass recognizes target-specific intrinsics at all is widely regarded as a mistake:
>  http://lists.llvm.org/pipermail/llvm-dev/2016-July/102317.html
>
> Target-specific transforms should look first at codegen combiners (SDAG or GlobalISel). If that's too late, consider a target-specific IR codegen pass (I think AMDGPU has a few examples of this already). If that's still too late, write a generic IR transform pass that accesses TTI?


The problem with all of these suggestions is that they're likely technically-inferior solutions compared to sitting inside of InstCombine's fixed-point iteration scheme. Honestly, I think that the way we should ensure that InstCombine does not start using TTI to define a canonical form for non-target-specific intrinsics is via documentation and code review. InstCombine has long had logic to deal with target-specific intrinsics (in InstCombineCalls.cpp), and refactoring things so that this logic can live in each backend seems like an improvement to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81728