selecting ISD node - help

Hey, I wanted to add an intrinsics to read MSRs.

So I added the intrinsics and lowered it to a new ISD node I created
ISD::RDMSR, its first operand is the MSR id.

I added a case in X86DAGToDAGISel::Select for ISD::RDMSR.
Now I know rdmsr works like so:
  mov r/ecx, <id>
  rdmsr
  r/eax holds the lower 32/64 bit

From what I understood this needs a Token Factor node, nodes which are

dependent on each other?
  case X86ISD::RDMSR:
  {
    unsigned idReg;
    SDValue idRegValue;
    unsigned resultReg;

    SDLoc dl = SDLoc(Node);

    SDValue id = Node->getOperand(0);
    EVT resultType = Node->getValueType(0);

    if(Subtarget->is64Bit())
    {
      idReg = X86::RCX;
      resultReg = X86::RAX;
    }
    else
    {
      idReg = X86::ECX;
      resultReg = X86::EAX;
    }

    idRegValue = CurDAG->getRegister(idReg, resultType);

    SmallVector<SDValue, 8> Ops;

    SDValue setIdNode = CurDAG->getCopyToReg(CurDAG->getEntryNode(), dl,
idRegValue, id, SDValue());
    SDValue rdmsrNode = SDValue(CurDAG->getMachineNode(X86::RDMSR, dl,
MVT::Other, setIdNode), 0);
    SDValue resultNode = CurDAG->getCopyFromReg(rdmsrNode, dl, resultReg,
resultType);

    Ops.push_back(resultNode);
    Ops.push_back(rdmsrNode);
    Ops.push_back(setIdNode);

    SDValue ResultValue = CurDAG->getNode(ISD::TokenFactor, dl, resultType,
&Ops[0], Ops.size());
    
    return ResultValue.getNode();
  }

But when I run this I get:
Assertion failed: I != VRBaseMap.end() && "Node emitted out of order -
late", file ..\..\..\..\lib\CodeGen\SelectionDAG\InstrEmitter.cpp, line 292

I know that I can just do this using in-line assembly instead of an
intrinsics but I want to learn a bit about LLVM development so I though
doing this (which I need anyway) will be a good way.
I'm pretty new to LLVM and I tried searching for the answer, but maybe I
misunderstood the answers or didn't find the correct post or code to look
at.
Help will be greatly appreciated. Thanks in advance.

Hi DaAn,

From what I understood this needs a Token Factor node, nodes which are
dependent on each other?

A TokenFactor probably isn't needed here. That's for when you have a
bunch of operations and you want to say to LLVM that you don't care
what order they happen in amongst themselves but certain things have
to happen after all of them have completed.

In this case, the most obvious analogue is probably division on x86,
which also has fixed registers (no doubt other instructions do too,
but that's the one I know about). If you compile a simple division
function:

define i32 @foo(i32 %a, i32 %b) {
  %res = sdiv i32 %a, %b
  ret i32 %res
}

with: llc simple.ll -view-sched-dags you should get a nice post-script
rendered view of what's going on.

You seem to have the most important bit already: the instruction
should be bracketed by copyToReg and copyFromReg nodes. I think the
bit that might be missing is that all three nodes should be connected
by what's called "glue" (drawn in red on that view-sched-dags output).

A "chain" (the dotted blue lines your code is attempting to use)
probably isn't enough because LLVM might feel free to put unrelated
instructions in the middle, which could clobber your registers --
there's no data-dependence as far as LLVM is concerned at this stage.

The code used for DIV is around X86ISelDAGToDAG.cpp:2415, but from a
glance the key points seem to be:
1. use the second result of getCopyToReg (i.e. SDValue(setIdNode, 1))
in the RDMSR node.
2. Give your RDMSR node type MVT::Glue instead of MVT::Other

You should then return the first output of the CopyFromReg node itself.

That said, none of this addresses your actual error message.
Fortunately, it looks like it's happening a bit later so I'd only
worry about it if it still occurred when the DAG was looking how I
expected (via -view-sched-dags).

Hope this helps. Let me know if you have more problems.

Tim.

Hi Tim,

Tim Northover-2 wrote

The code used for DIV is around X86ISelDAGToDAG.cpp:2415, but from a
glance the key points seem to be:
1. use the second result of getCopyToReg (i.e. SDValue(setIdNode, 1))
in the RDMSR node.
2. Give your RDMSR node type MVT::Glue instead of MVT::Other

I tried doing what you said, and the DAG looks like how I think it supposed
to look like (attached the picture below).

    if(Subtarget->is64Bit())
    {
      idReg = X86::RCX;
      resultReg = MF.addLiveIn(X86::RAX, &X86::GR64RegClass);
    }
    else
    {
      idReg = X86::ECX;
      resultReg = MF.addLiveIn(X86::EAX, &X86::GR32RegClass);
    }

    idRegValue = CurDAG->getRegister(idReg, resultType);

    SDValue setIdNode = CurDAG->getCopyToReg(CurDAG->getEntryNode(), dl,
idRegValue, id, SDValue());
    SDValue rdmsrNode = SDValue(CurDAG->getMachineNode(X86::RDMSR, dl,
MVT::Glue, setIdNode.getValue(1)), 0);
    SDValue resultNode = CurDAG->getCopyFromReg(CurDAG->getEntryNode(), dl,
X86::EAX, MVT::i32, rdmsrNode);
    
    return resultNode.getNode();

but I've a couple of problems:

getCopyToReg doesn't show on the final assembly
    __Z5test2v proc near
    push ebp
    mov ebp, esp
    rdmsr
    pop ebp
    retn
    __Z5test2v endp

When I add rdmsr + rdmsr, first the rdmsr calls are made and only then he
adds eax, eax.
    __Z5test3v proc near
    push ebp
    mov ebp, esp
    rdmsr
    rdmsr
    add eax, eax
    pop ebp
    retn
    __Z5test3v endp
Maybe it's because I use MF.addLiveIn but when I tried using X86::EAX it
failed with the message:
    # Machine code for function _Z5test2v: Post SSA

    BB#0: derived from LLVM BB %entry
            %vreg2<def> = COPY %EAX; GR32:%vreg2
            RDMSR
            %EAX<def> = COPY %vreg2; GR32:%vreg2
            RET %EAX<kill>

    # End machine code for function _Z5test2v.

    *** Bad machine code: Using an undefined physical register ***
    - function: _Z5test2v
    - basic block: BB#0 entry (0x2ff8228)
    - instruction: %vreg2<def> = COPY %EAX; GR32:%vreg2
    - operand 1: %EAX
    LLVM ERROR: Found 1 machine code errors.
    Stack dump:
    0. Program arguments:
C:\Users\DaAn\Documents\Projects\llvm-3.4\build\bin\Debug\llc.exe
-filetype=obj -view-sched-dags
C:\Users\DaAn\Documents\Projects\llvm-3.4\build\bin\Debug\test.bc
    1. Running pass 'Function Pass Manager' on module
'C:\Users\DaAn\Documents\Projects\llvm-3.4\build\bin\Debug\test.bc'.
    2. Running pass 'Greedy Register Allocator' on function '@_Z5test2v'

<http://llvm.1065342.n5.nabble.com/file/n65821/dag._Z5test2v-4ac530.png>
<http://llvm.1065342.n5.nabble.com/file/n65821/dag._Z5test3v-e16193.png>

Hi DaAn,

I tried doing what you said, and the DAG looks like how I think it supposed
to look like (attached the picture below).

I'm a little concerned that the CopyFromRegs are referencing "%vreg0"
rather than %EAX, but that might be an LLVM-internal thing (perhaps
because of your addLiveIns). Other than that they look right to me
too, so I wouldn't worry about it for now (your code looks OK).

getCopyToReg doesn't show on the final assembly

Ah, I think I may have just spotted the reason for that. The RDMSR
instruction hasn't been marked as using EDX:EAX or defining ECX
(incidentally, the Intel manual doesn't seem to make any mention of
RCX).

That's probably causing LLVM to drop the COPY instructions you're
generating (thinking they're dead code). I'd suggest changing the
X86InstrSystem.td:443 to be something like:

    let Defs = [ECX], Uses = [EAX, EDX] in
    def RDMSR : I<0x32, RawFrm, (outs), (ins), "rdmsr", [], IIC_RDMSR>, TB;

Maybe it's because I use MF.addLiveIn but when I tried using X86::EAX it
failed with the message:
          # Machine code for function _Z5test2v: Post SSA

          BB#0: derived from LLVM BB %entry
                  %vreg2<def> = COPY %EAX; GR32:%vreg2
                 RDMSR
                 %EAX<def> = COPY %vreg2; GR32:%vreg2

addLiveIn is definitely wrong (%eax isn't live upon entry to the
function). That snippet looks like the CopyFromReg and CopyToReg are
backwards to me. And the reuse of %vreg2 isn't inspiring confidence,
though it might be correct.

I've not looked in detail, but there's code in InstrEmitter.cpp
hinting that the ordering-constraints we want from the Glue may be
ignored if there isn't any register dependence, so I'd suggest fixing
the RDMSR definition and removing the addLiveIn together. With luck
everything will start working.

Cheers.

Tim.

    let Defs = [ECX], Uses = [EAX, EDX] in
    def RDMSR : I<0x32, RawFrm, (outs), (ins), "rdmsr", [], IIC_RDMSR>, TB;

Sorry, that's backwards. I think it should be:

     let Defs = [EAX, EDX], Uses = [ECX] in
     def RDMSR : I<0x32, RawFrm, (outs), (ins), "rdmsr", [], IIC_RDMSR>, TB;

Cheers.

Tim.