[PATCH] D150666: [WIP][GlobalISel] Combiner Intrinsic Matching

Fri May 19 11:21:21 PDT 2023

arsenm added inline comments.

================
Comment at: llvm/test/TableGen/GICombinerEmitter/match-tree.td:35

+def int_dummy : DefaultAttrsIntrinsic<[llvm_ptr_ty], [llvm_ptr_ty]>;
+let TargetPrefix = "foo" in
----------------
Pierre-vh wrote:
> arsenm wrote:
> > Should have one that has side effects and one that doesn't to cover both G_INTRINSIC* cases.
> > 
> > Also, maybe try one with multiple return values. Should theoretically be matchable 
> Will do, but about the G_INTRINSIC* case:
> Currently I match both G_INTRINSIC and G_INSTRINC_W_SIDE_EFFECTS for any intrinsic. Is that the correct behaviour?
> 
> I did a quick look around in the tests, and I think they're interchangeable and we can't use whether the intrinsic has side effects or not to deduce the opcode to use, right? It's just that the "normal" opcode can be removed if it's unused, but the second one cannot because it's considered to have side effects?
I'd say more no than yes. There's a narrow set of cases where you could conceivably use G_INTRINSIC based on callsite attributes for a normally G_INTRINSIC_W_SIDE_EFFECTS. The DAG does not support this case. Off the top of my head the only cases where we could care about this are because of the limitations of buffer load intrinsics 

TLDR not interchangeable 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150666