[mips][microMIPS] Prevent usage of OR16_MMR6 instruction when code for microMIPS is generated.

[sdardis]

There is a file test/CodeGen/Mips/micromips-or16.ll which can be used for this test rather than adding a new file.

[milena.vujosevic.janicic]

Test from last revision is integrated into test micromips-or16.ll, as suggested.

[dsanders]

lib/Target/Mips/MicroMips32r6InstrInfo.td

814–815 ↗

(On Diff #48326)

Can you put it on POOL16C_OR16_XOR16_FM_MMR6 instead?

(The other examples of MicroMipsR6Inst16 in this file are in the wrong place too)

test/CodeGen/Mips/micromips-or16.ll

3–43 ↗

(On Diff #48326)

This should be sufficient:

define i32 @f(i32 signext %a, i32 signext %b) {
  %1 = or i32 %a, %b
  ret i32 %1
}
; CHECK-LABEL: f:
; CHECK-NOT: OR16_MMR6

[milena.vujosevic.janicic]

I put MicroMipsR6Inst16 on POOL16C_OR16_XOR16_FM_MMR6, as you suggested.

The test-case you suggested would not work since it would always generate right OR16_MM instruction. However, I managed to simplify a bit the test case used last time.

The test case you suggested does not trigger this bug due to the way FastISel functions. It always first calls method FastISel::selectOperator which can do fast instruction selection for a small set of operators. Then, if selectOperator fails, FastISel calls MipsFastISel::fastSelectInstruction which checks if the architecture for which the code is generated is supported. If FastISel fails for one instruction, it does not continue trying for other instructions in that basic block. It selects instructions in reverse order.

In the test-case you suggested, FastISel tries to select an instruction for RET by selectOperator method and it fails. Then it tries to select an instruction by fastSelectInstruction, which again fails since microMips is not supported architecture, and then FastISel stops and instruction selection is continued in a normal way (and there is no bug).

In the test-case suggested in this patch, all instructions before OR are selected by selectOperator method, and then this method selects (wrong) OR16_MMR6 instruction.

[dsanders]

In D17373#364232, @milena.vujosevic.janicic wrote:

I put MicroMipsR6Inst16 on POOL16C_OR16_XOR16_FM_MMR6, as you suggested.

The test-case you suggested would not work since it would always generate right OR16_MM instruction. However, I managed to simplify a bit the test case used last time.

The test case you suggested does not trigger this bug due to the way FastISel functions. It always first calls method FastISel::selectOperator which can do fast instruction selection for a small set of operators. Then, if selectOperator fails, FastISel calls MipsFastISel::fastSelectInstruction which checks if the architecture for which the code is generated is supported. If FastISel fails for one instruction, it does not continue trying for other instructions in that basic block. It selects instructions in reverse order.

In the test-case you suggested, FastISel tries to select an instruction for RET by selectOperator method and it fails. Then it tries to select an instruction by fastSelectInstruction, which again fails since microMips is not supported architecture, and then FastISel stops and instruction selection is continued in a normal way (and there is no bug).

In the test-case suggested in this patch, all instructions before OR are selected by selectOperator method, and then this method selects (wrong) OR16_MMR6 instruction.

In that case we just need to split the bb in the small test case. The code below emits an OR16_MMR6 on my build.

test/CodeGen/Mips/micromips-or16.ll

3–43 ↗

(On Diff #49369)

This is the simplest test I can find that emits an OR16_MMR6:

define i32 @f(i32 signext %a, i32 signext %b) {
       %1 = or i32 %a, %b
       br label %b1
b1:
       ret i32 %1
}

[milena.vujosevic.janicic]

Simplified test case

[dsanders]

LGTM with main simplified as well.

test/CodeGen/Mips/micromips-or16.ll

4 ↗

(On Diff #49590)

Indentation is off by one

15–29 ↗

(On Diff #49590)

The allocas/loads/stores are unnecessary. We should simplify this in a similar way to 'f' (but with a single basic block).

llvm/trunk/lib/Target/Mips/MicroMips32r6InstrFormats.td

class POOL16C_NOT16_FM_MMR6 : MicroMipsR6Inst16 {
   bits<16> Inst;
 
   let Inst{15-10} = 0x11;
   let Inst{9-7}   = rt;
   let Inst{6-4}   = rs;
   let Inst{3-0}   = 0b0000;
 }
 
-class POOL16C_OR16_XOR16_FM_MMR6<bits<4> op> {
+class POOL16C_OR16_XOR16_FM_MMR6<bits<4> op> : MicroMipsR6Inst16 {
   bits<3> rt;
   bits<3> rs;
 
   bits<16> Inst;
 
   let Inst{15-10} = 0b010001;
   let Inst{9-7}   = rt;
   let Inst{6-4}   = rs;

llvm/trunk/lib/Target/Mips/MicroMips32r6InstrInfo.td

class LI16_MMR6_DESC : LoadImmMM16<"li16", li_simm7, GPRMM16Opnd>,
       MMR6Arch<"srl16">, MicroMipsR6Inst16, IsAsCheapAsAMove;
 class MOVE16_MMR6_DESC : MoveMM16<"move16", GPR32Opnd>, MMR6Arch<"srl16">,
       MicroMipsR6Inst16;
 class SDBBP16_MMR6_DESC : BrkSdbbp16MM<"sdbbp16">, MMR6Arch<"sdbbp16">,
       MicroMipsR6Inst16;
 class SUBU16_MMR6_DESC : ArithRMM16<"subu16", GPRMM16Opnd, 0, II_SUBU, sub>,
       MMR6Arch<"sdbbp16">, MicroMipsR6Inst16;
 class XOR16_MMR6_DESC : LogicRMM16<"xor16", GPRMM16Opnd, II_XOR, xor>,
-      MMR6Arch<"sdbbp16">, MicroMipsR6Inst16;
+      MMR6Arch<"sdbbp16">;
 
 class LW_MMR6_DESC : MMR6Arch<"lw">, MipsR6Inst {
   dag OutOperandList = (outs GPR32Opnd:$rt);
   dag InOperandList = (ins mem:$addr);
   string AsmString = "lw\t$rt, $addr";
   let DecoderMethod = "DecodeMemMMImm16";
   let canFoldAsLoad = 1;
   let mayLoad = 1;

llvm/trunk/test/CodeGen/Mips/micromips-or16.ll

 ; RUN: llc -march=mipsel -mcpu=mips32r2 -mattr=+micromips \
 ; RUN:   -relocation-model=pic -O3 < %s | FileCheck %s
+; RUN: llc -O0 -march=mips -mcpu=mips32r2 -mattr=+micromips \
+; RUN:   -asm-show-inst < %s | FileCheck %s
 
-define i32 @main() {
-entry:
-  %retval = alloca i32, align 4
-  %a = alloca i32, align 4
-  %b = alloca i32, align 4
-  %c = alloca i32, align 4
-  store i32 0, i32* %retval
-  %0 = load i32, i32* %b, align 4
-  %1 = load i32, i32* %c, align 4
-  %or = or i32 %0, %1
-  store i32 %or, i32* %a, align 4
-  ret i32 0
+; Branch instruction added to enable FastISel::selectOperator
+; to select OR instruction
+define i32 @f1(i32 signext %a, i32 signext %b) {
+; CHECK-LABEL: f1
+; CHECK-NOT: OR16_MMR6
+      %1 = or i32 %a, %b
+       br label %b1
+b1:
+       ret i32 %1
 }
 
+define i32 @f2(i32 signext %a, i32 signext %b) {
+entry:
+; CHECK-LABEL: f2
 ; CHECK: or16
+  %0 = or i32 %a, %b
+  ret i32 %0
+}