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

[Nikita Popov via Phabricator via cfe-commits]

nikic 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,
----------------
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?


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:542
+  bool simplifyDemandedUseBitsIntrinsic(InstCombiner &IC, IntrinsicInst &II,
+                                        APInt DemandedMask, KnownBits &Known,
+                                        bool &KnownBitsComputed,
----------------
`const APInt &DemandedMask`?


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:546
+  bool simplifyDemandedVectorEltsIntrinsic(
+      InstCombiner &IC, IntrinsicInst &II, APInt DemandedElts, APInt &UndefElts,
+      APInt &UndefElts2, APInt &UndefElts3,
----------------
`const APInt &DemandedElts`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81728