[PATCH] D91244: [GlobalISel] Add missing operand update when copy is required
Gabriel Hjort Ã…kerlund via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 4 23:01:12 PST 2021
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
More information about the llvm-commits
mailing list