[PATCH] D78137: [RegBankSelect] Hide assertion failure from LLT::getScalarSizeInBits

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In D78137#1984139 <https://reviews.llvm.org/D78137#1984139>, @dsanders wrote:

> In D78137#1982421 <https://reviews.llvm.org/D78137#1982421>, @vsk wrote:
>
> > In D78137#1982359 <https://reviews.llvm.org/D78137#1982359>, @dsanders wrote:
> >
> > > > (lldb) p MF.dump()
> > > > 
> > > > 1. Machine code for function phiPropagation: IsSSA, TracksLiveness, Legalized
> > > > 
> > > >   bb.0.entry: successors: %bb.2(0x40000000), %bb.1(0x40000000); %bb.2(50.00%), %bb.1(50.00%) liveins: $x0, $x1, $w2 %0:gpr32 = LDRWui killed $x0, 0, debug-location !23 :: (load 4 from %ir.src); /Users/vsk/src/llvm-project-master/llvm/test/CodeGen/AArch64/GlobalISel/arm64-regbankselect.mir:6:1 DBG_VALUE %0:gpr32, $noreg, !"1", !DIExpression(), debug-location !23; /Users/vsk/src/llvm-project-master/llvm/test/CodeGen/AArch64/GlobalISel/arm64-regbankselect.mir:6:1 line no:6 ```
> > >
> > > If you moved the DBG_VALUE below the `%5(s32) = COPY %0` does it still assert? It's interesting that the target instruction means the DBG_VALUE is the first to be processed in that def/use chain but I suspect the real problem is that it's not deriving the register bank from the register class and the special case for COPY deals with that.
> >
> >
> > No, after moving the DBG_VALUE, I don't hit the assertion:
> >
> >   %0:gpr32 = LDRWui
> >   %5:_(s32) = COPY %0
> >   DBG_VALUE %5
> >
> >
> > Sorry, I'm not entirely sure what to make of that. Do you suspect -mir-debugify is adding DBG_VALUE insts in the wrong place? In the generic regbankselect path, is it required for operands to go through a COPY before they can be used?
>
>
> I think mir-debugify is doing the right thing (at the very least it's doing a valid thing) and RegBankSelect is wrong to fail on it. In GlobalISel, vregs can have three kinds of constraint: LLT, Register Bank, Register Class. There's a relationship between the type of constraint that should be used and the kind of instruction they're used on. For example, a generic opcode (G_*) should have either LLT or Register Bank constraints but not a Register Class constraint. The important one is that target instructions should have a Register Class constraint.
>
> What I believe is happening without this patch is it's seeing DBG_VALUE isn't a target instruction and concluding that it will never have a Register Class constraint and must therefore either be assigned a new Register Bank constraint or an existing Register Bank constraint must be read to influence the decisions elsewhere in the algorithm. However, when the DBG_VALUE occurs before the COPY that assumption is wrong and the code asserts on the unexpected Register Class constraint.
>
> The simplest fix to this is to just skip the DBG_VALUE's as you've done in this patch. We'll still make a decision based on the other instructions that def/use the vreg and the DBG_VALUE shouldn't be influencing the decisions anyway.


Thanks for your patient explanation! All right, I'll consider this good to land then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78137