[PATCH] D91244: [GlobalISel] Add missing operand update when copy is required

[Gabriel Hjort Åkerlund via Phabricator via llvm-commits]

ehjogab added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/InstructionSelector.cpp:47
+  if (NewReg != MO.getReg()) {
+    MO.setReg(NewReg);
+  }
----------------
arsenm wrote:
> Does this need to call the change observer? Is a test possible?
I'm not familiar with the change observer, so I don't know whether it needs to be called. If you know, can you please inform me on how the change observer works and is intended to be used?

The test case requires a target where you have a register bank that can map to at least two register classes, and I haven't found any target with that. Due to how target is currently implemented, we use a single register bank as an intermediary step in porting our own target to global-isel, and that's when I stumbled upon this bug.

To convince oneself that this is a bug, look at how constrainOperandRegToRegClass() is called. There is only one place, in the implementation of GIR_ConstrainOperandRC in llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h. There, it does nothing to update the instruction in case a copy is emitted. Also, the documentation for constrainOperandRegToRegClass() says the following (see bold):

  /// Constrain a register operand of an instruction \p I to a specified 
  /// register class. This could involve inserting COPYs before (for uses) or
  /// after (for defs) and **may replace the operand of \p I.**
  /// \returns whether operand regclass constraining succeeded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91244