Trouble adding LLVM intrinsic for custom RISCV hardware

Code Generation RISCV
,
1

Hi,

I’m trying to add a new intrinsic to llvm (skipping clang builtins for now) for custom RISCV hardware we have developed and I’m having issues compiling clang. I’m new to compiler development and this is my time in this forum so bear with me if I’m being a little slow.

First, I added an intrinsic that does not have inputs but does have an output register and that seems to compile just fine. However, when I added another one the code stopped compiling. The intrinsic in question will execute an instruction that takes in 2 registers as inputs and has no outputs but it will modify a value in memory. The code I added is as follows:

// llvm/include/llvm/IR/IntrinsicsRISCV.td

...
def int_riscv_first :
        Intrinsic<[llvm_anyfloat_ty], [], [IntrReadMem]>;
def int_riscv_second :
        Intrinsic<[], [llvm_anyfloat_ty, LLVMMatchType<0>], [IntrWriteMem, IntrHasSideEffects]>;
...

// llvm/lib/Target/RISCV/RISCVInstrInfoCustom.td

...
def SDTCustomLeaf: SDTypeProfile<1, 0, [SDTCisVT<0, f32>]>;
def SDTCustomOp: SDTypeProfile<0, 2, [SDTCisSameAs<1, 2>, SDTCisVT<1, f32>]>;
...
def riscv_first
    : SDNode<"RISCVISD::FIRST", SDTCustomLeaf, [SDNPHasChain]>;
def riscv_second
    : SDNode<"RISCVISD::SECOND", SDTCustomOp, [SDNPHasChain]>;
...
def FIRST_S : ...,
               Sched<[]>{
  let rs1 = 0b00000;
  let rs2 = 0b00000;
}
def SECOND_S : ...,
               Sched<[]>{
  let rd = 0b00000;
}
...
def : Pat<(riscv_first), (FIRST_S)>;
// PosR32 is a custom float register bank
def : Pat<(riscv_second PosR32:$rs1, PosR32:$rs2), (SECOND_S PosR32:$rs1, PosR32:$rs2)>;

// llvm/lib/Target/RISCV/RISCVISelLowering.h

...
namespace RISCVISD {
enum NodeType : unsigned {
    ...
    FIRST,
    SECOND,
    ...
}
...

// llvm/lib/Target/RISCV/RISCVISelLowering.cpp

...
SDValue RISCVTargetLowering::LowerINTRINSIC_WO_CHAIN(SDValue Op, SelectionDAG &DAG) const {
    unsigned IntNo = Op.getConstantOperandVal(0);
    SDLoc DL(Op);
    MVT XLenVT = Subtarget.getXLenVT();
    switch (IntNo) {
    ...
    case Intrinsic::riscv_first: {
        SDLoc DL(Op);
        return DAG.getNode(RISCVISD::FIRST, DL, MVT::Other, Op->getOperand(0));
    }
    }
    ...
 }
...
SDValue RISCVTargetLowering::LowerINTRINSIC_VOID(SDValue Op, SelectionDAG &DAG) const {
    unsigned IntNo = Op.getConstantOperandVal(1);
    switch (IntNo) {
    ...
    case Intrinsic::riscv_second: {
        SDLoc DL(Op);
        return DAG.getNode(RISCVISD::SECOND, DL, MVT::Other, Op->getOperand(1), Op->getOperand(2));
    }
    }
    ...
}
...
switch ((RISCVISD::NodeType)Opcode) {
...
NODE_NAME_CASE(FIRST)
NODE_NAME_CASE(SECOND)
...
}
...

The error this returns when compiling is
error: Invalid operand number in type constraint 2 (riscv_second PosR32:{ *:[f32] }:$rs1, PosR32:{ *:[f32] }:$rs2)

I know there are probably a lot of issues here but what I am not really sure about is why when I it’s only the second intrinsic that does not compile. I probably doing something fundamentally wrong here but I could not find anything useful on the documentation.

2

The SDTCisSameAs<1, 2> has 2 in it, but the operand numbers can only be 0 and 1.

3

That makes sense. I assumed 0 was always for the output, but it makes sense that is will start at the first one that actually exists.

4

It compiles! Thanks a lot.

Another question I had is whether I can (and should) add a custom type. Currently I am using f32 but ideally this would not be a float. I’ve seen that I cannot add a new SDTypeConstraint based on the comments in TargetSelectionDAG.td. Does that mean I should not add a new type and use f32 instead?

5

You should be able to use any legal type from MVT. Were you planning to add your own type to MVT?

6

Yes, is that a good idea?

7

There is no simple answer to that. It depends on specific circumstances. If you’re adding it to work around some issue, then probably not, if it you want it because it reflect some property of the target that is not yet represented, then perhaps. I guess one test would be whether you think you could justify adding the new type in the upstream repo.