[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:
> 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)
I find three places where constrainOperandRegClass() is called and its result is used to update the operand. So I disagree with the original intent being that constrainOperandRegClass() should also update the operand. There is also a constrainOperandRegClass() for SelectionISel and there the result is sometimes used to update the operand, so I believe that the behavior for GlobalISel was simply copied from how SelectionISel does it.

But I also agree that constrainOperandRegToRegClass() is quite useless, and since all cases where constrainOperandRegClass() is invoked the result is used to update the operand, maybe it is a better idea to let constrainOperandRegClass() do it for us. I will update the patch accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91244