[PATCH] D87117: [GlobalISel] Add G_UNMERGE_VALUES(G_MERGE_VALUES) combine

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:392
+>;
+
 // FIXME: These should use the custom predicate feature once it lands.
----------------
arsenm wrote:
> Missing from all combines
@arsenm What do you mean?

I've added it to `all_combines` already, did I miss something else?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1561
+                                   const MachineRegisterInfo &MRI) {
+  while (mi_match(Reg, MRI, m_GBitcast(m_Reg(Reg))))
+    ;
----------------
arsenm wrote:
> arsenm wrote:
> > Since this uses buildCast, this can accept a wider array of cast types (particularly G_PTRTOINT and G_INTTOPTR?)
> Actually I was thinking there should some combines to push casts through merges instead?
G_PTRTOINT and G_INTTOPTR may not be no-ops (they may change the size of the types), so I didn't think we wanted to have them here.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1561
+                                   const MachineRegisterInfo &MRI) {
+  while (mi_match(Reg, MRI, m_GBitcast(m_Reg(Reg))))
+    ;
----------------
qcolombet wrote:
> arsenm wrote:
> > arsenm wrote:
> > > Since this uses buildCast, this can accept a wider array of cast types (particularly G_PTRTOINT and G_INTTOPTR?)
> > Actually I was thinking there should some combines to push casts through merges instead?
> G_PTRTOINT and G_INTTOPTR may not be no-ops (they may change the size of the types), so I didn't think we wanted to have them here.
> Actually I was thinking there should some combines to push casts through merges instead?

The artifact combiner does that, but AFAICT the combiner helper doesn't have such a thing. At least not yet.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1605
+  bool CanReuseInputDirectly = DstTy == SrcTy;
+  Builder.setInstr(MI);
+  for (unsigned Idx = 0; Idx < NumElems; ++Idx) {
----------------
arsenm wrote:
> setInstrAndDebugLoc? (We should probably just make setInstr always set the debug location)
Oh, I didn't know about that one. Changing!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87117