llvm dev - Jun 2019 - How to handle ISD::STORE when both operands are FrameIndex?

On Wed, Jun 26, 2019 at 12:38 PM Tim Northover <t.p.northover at
gmail.com>
wrote:
> Hi Gleb,
>
> On Wed, 26 Jun 2019 at 07:28, Gleb Popov <6yearold at gmail.com>
wrote:
> > def StoreStackF : InstRI<2, (outs), (ins IntRegs:$reg, i32imm:$i),
> >                     "storestackf $reg, [$i]", [(store_stack
i32:$reg,
> AddrFI:$i)]>;
> >
> > I'm puzzled why despite having "IntRegs:$reg" in ins
list, it still
> matches FrameIndex:
>
> A register-class will match anything with the appropriate type (i32
> here). The idea is that any value of that type can be put into a
> register somehow, and matching whatever it happens to be (a FrameIndex
> in this case) is exactly what's needed to do that.
>
That's a useful explanation, thanks.

> > ISEL: Starting selection on root node: t11: i32 = FrameIndex<2>
> > ISEL: Starting pattern match
> >   Initial Opcode index to 0
> >   Match failed at index 0
> > LLVM ERROR: Cannot select: t11: i32 = FrameIndex<2>
>
> Ah good, this is what I thought was probably happening so it's nothing
> exotic. You need to be able to match a FrameIndex somehow. Not just
> for this, but for something like:
>
>     void foo() {
>       int arr[5];
>       bar(&arr[0]);
>     }
>
> It's pretty simple to implement matching, you generally just need to
> convert it to some add instruction that can do SP+imm.

For the sake of simplicity I decided not to bother with stack register yet
and instead I just emit FrameIndex as immediate:

bool MyBackendDAGToDAGISel::SelectAddrFI(SDValue &N, SDValue &R) {
  if (N.getOpcode() != ISD::FrameIndex)
    return false;
  MachineFrameInfo &MFI = MF->getFrameInfo();
  int FX = cast<FrameIndexSDNode>(N)->getIndex();
  R = CurDAG->getTargetFrameIndex(FX, MVT::i32);
  return true;
}

This way I end up with

store %r1, [1]

and handle it in my CPU emulator accordingly.

So, instead of matching that FrameIndex in store, I really want to emit a
load first and then use a register in the store instruction.
Can you, please, advise me how to implement that?

See for example
> lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2916.
>
> Cheers.
>
> Tim.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190626/0b6684d8/attachment.html>

On Wed, 26 Jun 2019 at 10:27, Gleb Popov <6yearold at gmail.com>
wrote:
> For the sake of simplicity I decided not to bother with stack register yet
and instead I just emit FrameIndex as immediate:
>
> bool MyBackendDAGToDAGISel::SelectAddrFI(SDValue &N, SDValue &R) {
>   if (N.getOpcode() != ISD::FrameIndex)
>     return false;
>   MachineFrameInfo &MFI = MF->getFrameInfo();
>   int FX = cast<FrameIndexSDNode>(N)->getIndex();
>   R = CurDAG->getTargetFrameIndex(FX, MVT::i32);
>   return true;
> }
>
> This way I end up with
>
> store %r1, [1]
>
> and handle it in my CPU emulator accordingly.
>
> So, instead of matching that FrameIndex in store, I really want to emit a
load first and then use a register in the store instruction.
As you say, SelectAddrFI will only be used when looking into a store,
and I think that's fine: it's a specialized addressing-mode
implemented in just the right place. You need a completely separate
"case" in MyBackendDAGToDAGISel::Select to handle a bare
ISD::FrameIndex.

Since you don't use SP, your implementation of FrameIndex will
probably just be a MOV-immediate instruction. Final output would be
something like:

    mov %r1, 2     // Produced by matching the ISD::FrameIndex
    store %r1, [1]

I don't think load is going to be involved anywhere unless you meant
it in the broader sense where (e.g.) Z80 referred to its move
instructions as loads.

Cheers.

Tim.

On Wed, Jun 26, 2019 at 1:37 PM Tim Northover <t.p.northover at gmail.com>
wrote:
> On Wed, 26 Jun 2019 at 10:27, Gleb Popov <6yearold at gmail.com>
wrote:
> > For the sake of simplicity I decided not to bother with stack register
> yet and instead I just emit FrameIndex as immediate:
> >
> > bool MyBackendDAGToDAGISel::SelectAddrFI(SDValue &N, SDValue
&R) {
> >   if (N.getOpcode() != ISD::FrameIndex)
> >     return false;
> >   MachineFrameInfo &MFI = MF->getFrameInfo();
> >   int FX = cast<FrameIndexSDNode>(N)->getIndex();
> >   R = CurDAG->getTargetFrameIndex(FX, MVT::i32);
> >   return true;
> > }
> >
> > This way I end up with
> >
> > store %r1, [1]
> >
> > and handle it in my CPU emulator accordingly.
> >
> > So, instead of matching that FrameIndex in store, I really want to
emit
> a load first and then use a register in the store instruction.
>
> As you say, SelectAddrFI will only be used when looking into a store,
> and I think that's fine: it's a specialized addressing-mode
> implemented in just the right place. You need a completely separate
> "case" in MyBackendDAGToDAGISel::Select to handle a bare
> ISD::FrameIndex.
>
Thanks for clearing this up! I was thinking the same, but wasn't sure.

> Since you don't use SP, your implementation of FrameIndex will
> probably just be a MOV-immediate instruction. Final output would be
> something like:
>
>     mov %r1, 2     // Produced by matching the ISD::FrameIndex
>     store %r1, [1]
>
> I don't think load is going to be involved anywhere unless you meant
> it in the broader sense where (e.g.) Z80 referred to its move
> instructions as loads.
>
Silly me -_\
Why was I talking about a load all this time? You're right, I needed a
simple mov there. I have added the following code into
MyBackendDAGToDAGISel::Select() and it worked:

if(FrameIndexSDNode *FIN = dyn_cast<FrameIndexSDNode>(N))
  {
      auto Addr = CurDAG->getTargetFrameIndex(
                    FIN->getIndex(),
TLI->getPointerTy(CurDAG->getDataLayout()));
      SDNode *MOV = CurDAG->getMachineNode(MYBACK::MovRI, dl, MVT::i32,
Addr);
      CurDAG->ReplaceAllUsesWith(N, MOV);
      return;
  }

I'm eternally grateful for helping me with this, thank you again.

> Cheers.
>
> Tim.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190628/335a9ee3/attachment.html>