[PATCH] D18315: [mips] MIPSR6 Compact jump support

[Vasileios Kalintiris via llvm-commits]

vkalintiris added a comment.

Hi Simon, I added some comments inline.


================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:88
@@ +87,3 @@
+  bit isTerminator = 1;
+  list<Register> Defs = [AT];
+}
----------------
I know that you probably based the definition on the 32-bit description, but do you know why we have to define $at?

================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:122-125
@@ -109,3 +121,6 @@
 }
-
+let isCodeGenOnly = 1 in {
+def JIALC64 : JIALC_ENC, JIALC64_DESC, ISA_MIPS64R6;
+def JIC64 : JIC_ENC, JIC64_DESC, ISA_MIPS64R6;
+}
 //===----------------------------------------------------------------------===//
----------------
Can you add the relevant MC tests for the new instructions? Either in a new review request or in this one would be fine with me.

================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:260
@@ +259,3 @@
+/// True if the given CTI has a compact form available in microMIPS.
+bool MipsInstrInfo::CTIHasCompactMMEncoding(
+    const MachineBasicBlock::iterator I) const {
----------------
Can we merge the functionality of this into `getEquivalentCompactForm()`? As far as I can tell, there's no other user of this member function and there's no point in duplicating the switch statement for every opcode twice.

================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:390-396
@@ -349,9 +389,9 @@
   // branch distance in non-microMIPS mode.
-  if (I->isBranch() && I->getOperand(1).isReg() &&
+  if (I->isBranch() && !I->isPseudo() && I->getOperand(1).isReg() &&
       // FIXME: Certain atomic sequences on mips64 generate 32bit references to
       // Mips::ZERO, which is incorrect. This test should be updated to use
       // Subtarget.getABI().GetZeroReg() when those atomic sequences and others
       // are fixed.
       (I->getOperand(1).getReg() == Mips::ZERO ||
        I->getOperand(1).getReg() == Mips::ZERO_64)) {
     BranchWithZeroOperand = true;
----------------
I suggest that we move most of these conditions into the initialization of `BranchWithZeroOperand` in order to simplify the code.

================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:411-413
@@ -370,5 +410,5 @@
       break;
     case Mips::BNEZC_MM:
     case Mips::BEQZC_MM:
       break;
     default:
----------------
Why do we need these cases? We don't update the NewOpc when they match.

================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:422-424
@@ -381,2 +421,5 @@
 
-  for (unsigned J = 0, E = I->getDesc().getNumOperands(); J < E; ++J)
+  if (NewOpc == Mips::JIALC || NewOpc == Mips::JIALC64) {
+    MIB->RemoveOperand(0);
+  }
+
----------------
What is going to happen to the defined register RA{,_64}, 

================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:422-436
@@ -381,5 +421,17 @@
 
-  for (unsigned J = 0, E = I->getDesc().getNumOperands(); J < E; ++J)
+  if (NewOpc == Mips::JIALC || NewOpc == Mips::JIALC64) {
+    MIB->RemoveOperand(0);
+  }
+
+  for (unsigned J = 0, E = I->getDesc().getNumOperands(); J < E; ++J) {
     if (!(BranchWithZeroOperand && (J == 1)))
       MIB.addOperand(I->getOperand(J));
+  }
+
+  if (NewOpc == Mips::JIC || NewOpc == Mips::JIALC || NewOpc == Mips::JIC64 ||
+      NewOpc == Mips::JIALC64) {
+    MIB.addOperand(MachineOperand::CreateImm(0));
+  }
+
+  MIB.copyImplicitOps(*I);
 
----------------
vkalintiris wrote:
> What is going to happen to the defined register RA{,_64}, 
I believe that we should simplify this by handling the logic in three separate cases: (1) JIALC et al., (2) BranchWithZeroOperand == true, and (3) everything else. I'm afraid that this code could become very complicated in time if we consider all the ABIs & ISAs that we have to support.

================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:433
@@ +432,3 @@
+      NewOpc == Mips::JIALC64) {
+    MIB.addOperand(MachineOperand::CreateImm(0));
+  }
----------------
We can use the convenient MIB.addImm() here.

================
Comment at: test/CodeGen/Mips/llvm-ir/ret.ll:21-25
@@ +20,7 @@
+
+; FIXME: for some reason, the delay filler kicks in test ret_double_0x0 for
+;        mips64r6 but doesn't for all others. I suspect that the mthc1 is
+;        misdefined somehow, as it should be able to go in the delay slot
+;        of jr. dmtc1 fills the delay slot safely for this test, but many other
+;        cases the delay slot is unfilled. Force it off for this test.
+
----------------
MTHC1 has *unmodeled side-effects* and the DSF bails out on these instructions (see Filler::terminateSearch). Can you update the comment to reflect this?

================
Comment at: test/MC/Mips/mips32r6/valid.s:166-167
@@ -165,2 +165,4 @@
         jalr.hb $4, $5           # CHECK: jalr.hb $4, $5         # encoding: [0x00,0xa0,0x24,0x09]
+ 	jialc	$15, 16161       # CHECK: jialc $15, 16161       # encoding: [0xf8,0x0f,0x3f,0x21]
+    	jic	$12, -3920       # CHECK: jic $12, -3920         # encoding: [0xd8,0x0c,0xf0,0xb0]
         ldc2    $8, -701($at)    # CHECK: ldc2 $8, -701($1)      # encoding: [0x49,0xc8,0x0d,0x43]
----------------
Indentation.


http://reviews.llvm.org/D18315