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

[Sebastian Neubauer via Phabricator via llvm-commits]

Flakebi marked an inline comment as done.
Flakebi added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:540
+  bool instCombineIntrinsic(InstCombiner &IC, IntrinsicInst &II,
+                            Instruction **ResultI) const;
+  bool simplifyDemandedUseBitsIntrinsic(InstCombiner &IC, IntrinsicInst &II,
----------------
nikic wrote:
> For all three functions, the calling convention seems rather non-idiomatic for InstCombine. Rather than having an `Instruction **` argument and bool result, is there any reason not to have an `Instruction *` return value, with nullptr indicating that the intrinsic couldn't be simplified?
Yes, the function must have the option to return a nullptr and prevent that `visitCallBase` is called or other code is executed after `instCombineIntrinsic`.
So, somehow the caller must be able to see a difference between 'do nothing, just continue execution' and 'return this Instruction*', where the `Instruction*` can also be a nullptr.
The return type could be an `optional<Instruction*>`.

I’ll take a look at your other comments on Monday.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81728