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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/InstructionSelector.cpp:47
+  if (NewReg != MO.getReg()) {
+    MO.setReg(NewReg);
+  }
----------------
ehjogab wrote:
> 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.
Looking at how constrainOperandRegClass is implemented, I think the intent was it would handle all of the instruction modifications. It should be responsible for updating the operand, not the caller here (actually InstructionSelector::constrainOperandRegToRegClass seems like a fairly pointless wrapper function)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91244