[PATCH] D70564: [GlobalISel] LegalizationArtifactCombiner: Fix a bug in tryCombineMerges

[Volkan Keles via Phabricator via llvm-commits]

volkan marked 2 inline comments as done.
volkan added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h:273
+  /// depending on the register constraints.
+  static void replaceRegOrBuildCopy(Register DstReg, Register SrcReg,
+                                    MachineRegisterInfo &MRI,
----------------
Petar.Avramovic wrote:
> Where will we use replaceRegOrBuildCopy so it has to be static?
> I think that eventually Observer should end up as field in LegalizationArtifactCombiner like in LegalizerHelper. There are a few `LLVM_DEBUG(dbgs() << ".. Combine MI: " << MI;);` around and Observer could handle them.
I'm not sure if we need to have a separate function for this, I added it based on the initial review. I can move this logic into `tryCombineMerges` as an alternative. Regarding the observer, it makes sense to pass the observer to simplify the functions, but it probably shouldn't be a part of this change. Also, I'm not sure if we still want to keep LegalizationArtifactCombiner as discussed in D65894.


================
Comment at: llvm/test/CodeGen/Mips/GlobalISel/regbankselect/long_ambiguous_chain_s32.mir:3-6
+
+; XFAIL: *
+; MipsRegisterBankInfo is unable to handle COPY instructions.
+
----------------
Petar.Avramovic wrote:
> With updated patch there are no changes in regbankselect tests, you can remove XFAIL. Register we are replacing in combine has no constraints.
I'll do that, thanks!


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

https://reviews.llvm.org/D70564