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

Daniel Sanders via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 7 12:35:03 PST 2021


dsanders added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/Utils.h:71-73
+/// correct class and insert a COPY before \p InsertPt if it is a use or after
+/// if it is a definition. In both cases, the function also updates the register
+/// of RegMo.
----------------
Just to mention it, there may be some consequences to adding 'or after' to this. I don't know if it's still the case or if we fixed them all but some passes used to be a bit picky on exactly where it was safe to insert instructions around the 'current' instruction and would fail to act on instructions or crash due to invalidated iterators or newly freed instructions.


================
Comment at: llvm/lib/CodeGen/GlobalISel/InstructionSelector.cpp:47
+  if (NewReg != MO.getReg()) {
+    MO.setReg(NewReg);
+  }
----------------
ehjogab wrote:
> 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.
> 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?

If you change the MIR (or are about to in some cases like erasingInstr()), you need to tell the observers. The observer is a way for something (typically the core loop of a pass) to keep track of what's going on so it can maintain it's data structures (e.g. avoid visiting deleted instructions), or report/analyze it. Other observers analyze lost DebugLoc's or simply print debugging output

Some observers can be fairly tolerant to failing to call them (especially the changingInstr/changedInstr functions as they were added later) but it's still a bug not to.

> 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.

FWIW, it doesn't necessarily need to update the instruction immediately here but it's not doing the other method either. The other method is to have a subsequent GIR_* opcode that updates the uses but that would require us to record the result somewhere and we don't. I think the right method is to update as part of the GIR_ConstrainOperandRC as the other one will bloat the state machine a lot to handle a fairly rare case.

> actually InstructionSelector::constrainOperandRegToRegClass seems like a fairly pointless wrapper function

Yep, it looks like it's no longer needed. A couple years back, tablegen emitted C++ directly and it was providing a lot of value for compile-time of the compiler itself. Eventually the compile-time of the generated code got excessive and we implemented the state machine

> 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. 

It's a bit of both really. The intent is for the caller to update the instruction it was working on and for the callee to update any other instructions that needed to be changed because of the constraint. The reason for the division is that the caller might be making other changes too that it either already has or is about to report. It's a bug to call changedInstr() and then make further changes without a new changingInstr() and it's undesirable to over-report changes by calling changingInstr()/changedInstr() multiple times for one change.


================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:76-84
     if (GISelChangeObserver *Observer = MF.getObserver()) {
       if (!RegMO.isDef()) {
         MachineInstr *RegDef = MRI.getVRegDef(Reg);
         Observer->changedInstr(*RegDef);
       }
       Observer->changingAllUsesOfReg(MRI, Reg);
       Observer->finishedChangingAllUsesOfReg();
----------------
ehjogab wrote:
> arsenm wrote:
> > I think I misread this the first time through and thought this was replacing uses. However I'm still a bit confused. 
> > 
> > This calls changingAllUsesOfReg after the potential change from constrainRegToClass (this will also happen even if constrainRegToClass ends up being a no-op when the register already was a compatible class). I'm further confused since the existing cases get away without calling the observer. I'm guessing this is because this is already called in contexts changing the instruction, or not bothering with the observer? If we don't need to call the observer here, I would prefer the first version but I'm unclear on whether it's really needed here at all
> This functions may replace both uses as well as defs, and the bug this patch was originally intended to fix concerned lack of update for defs.
> 
> I agree that things are not clear with the observer is intended to be called and when it is strictly necessary to do so. It would be good to have someone with such knowledge to have a look at this patch. 
I've commented on this above (which I think is the same thread and phabricator has just failed to track it properly)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91244



More information about the llvm-commits mailing list