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

Mon May 22 13:58:03 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:
> > 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 
> Are there any case where they could be interchangeable or not? 
> 
> If not then I can just make the Intrinsic predicate a subclass of the "Opcode" predicate instead of "OneOfOpcode" predicate. It'll also fix one of my 2 issues which is duplicated code for both G_INTRINSIC instructions. 
> 
> However if we think we may want to be able to match both G_INTRINSIC opcode for one pattern, then I can leave it in. I wouldn't be surprised if we see more G_INTRINSIC variants pop up so maybe there'll be a case down the line to handle 2+ opcodes for some kinds of intrinsics.
> 
> 
The only case would be if we want to use callsite attributes to optimize opcode properties, but the only cases where we might want that are kind of workarounds. I wouldn't try to support that now 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150666