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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh 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
----------------
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?


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp:462
+
+        // FIXME: This is needed, but prevents patterns such as
+        //   (match (G_FPTRUNC $fptrunc_dst, $fmed3_dst):$fptrunc,
----------------
arsenm wrote:
> Pierre-vh wrote:
> > This is the most annoying bit at the moment. It's quite restrictive and the example in the github issue doesn't work because of it: https://github.com/llvm/llvm-project/issues/62628
> Something is wrong with this "fully tested" code. It seems to just be wrong and mis-reports errors for cases which do match in the generated code (which is properly produced0
Indeed, I think it's not entirely right. I will take a deeper look at it but I also hope @dsanders can help here :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150666