This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Add CodeGen support for ADD, ADDIU*, ADDU* and DADD* instructions
ClosedPublic

Authored by zbuljan on Jan 22 2016, 1:33 AM.

Details

Summary

The patch adds CodeGen support for microMIPS32r6 ADD, ADDIU*, ADDU* and microMIPS64r6 DADD* instructions:

  • updated add.ll with tests for microMIPSr6 ADD, ADDIU*, ADDU* and DADD* instruction selection
  • separated microMIPS64r6 DADD* instructions from equivalent MIPS64 instructions using NotInMicroMips predicate
  • added DAG pattern to ADDIU description class for proper selection of the instruction
  • implemented DADD, DADDIU and DADDU instructions for microMIPS64r6 and added tests for the standard encodings

Diff Detail

Event Timeline

zbuljan updated this revision to Diff 45661.Jan 22 2016, 1:33 AM
zbuljan retitled this revision from to [mips][microMIPS] Add CodeGen support for ADD, ADDIU*, ADDU* and DADD* instructions.
zbuljan updated this object.
zbuljan added subscribers: petarj, llvm-commits.
dsanders added inline comments.Feb 17 2016, 8:06 AM
lib/Target/Mips/MicroMips64r6InstrInfo.td
185–195

Indentation on the $imm operands.

lib/Target/Mips/Mips64InstrInfo.td
542–545

These used to be disabled for any DSP, not just microMIPS DSP. Is there a reason for the change?

zbuljan added inline comments.Feb 18 2016, 12:45 AM
lib/Target/Mips/Mips64InstrInfo.td
542–545

I wanted do disable these for microMIPS also so I introduced new ISA_NOT_MICROMIPS_DSP class with list of predicates: NotInMicroMips and NotDSP.
Maybe it would be better to name class as ISA_NOT_MICROMIPS_NOT_DSP?

dsanders added inline comments.Feb 19 2016, 2:49 AM
lib/Target/Mips/Mips64InstrInfo.td
542–545

Ah ok, it's just the name that's a bit misleading.

I think the change you want is

let AdditionalPredicates = [NotInMicroMips,NotDSP] in {

This is because predicates belong to a specific group of predicates (e.g. EncodingPredicates, InsnPredicates, etc.) and both of these belong to AdditionalPredicates at the moment. That said, it would be easy to change NotDSP into an InsnPredicate since there's only three uses of it, then you could have

let AdditionalPredicates = [NotInMicroMips] in {
  def : MipsPat<(addc GPR64:$lhs, GPR64:$rhs),
              (DADDu GPR64:$lhs, GPR64:$rhs)>, ASE_NOT_DSP;

which is more consistent with other code.

At some point we need to move NotInMicroMips into the EncodingPredicates group.

zbuljan updated this revision to Diff 50030.Mar 8 2016, 3:54 AM
zbuljan edited edge metadata.

Rebased to work with top of the tree.
Added ASE_NOT_DSP class.

dsanders accepted this revision.Apr 4 2016, 8:03 AM
dsanders edited edge metadata.

LGTM

This revision is now accepted and ready to land.Apr 4 2016, 8:03 AM
This revision was automatically updated to reflect the committed changes.

Post commit comments:

I noticed that MipsSEDAGToDAGISel::selectNode(SDNode *Node) performs the expansion of ISD::ADDE. Unfortunately the logic there was not extended to cover microMIPS:

case ISD::ADDE: {
  if (Subtarget->hasDSP()) // Select DSP instructions, ADDSC and ADDWC.
    break;
  SDValue InFlag = Node->getOperand(2);
  unsigned Opc = Subtarget->isGP64bit() ? Mips::DADDu : Mips::ADDu; // Here
  Result = selectAddESubE(Opc, InFlag, InFlag.getValue(0), DL, Node);
  return std::make_pair(true, Result);
}

microMIPSR6 re-encodes 'addu', so I believe direct object emission may be broken.

My previous comment was made a bit too quickly. We have tables that map MIPS(R6) instructions to microMIPS(R6) instructions.