This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by milena.vujosevic.janicic on Feb 18 2016, 2:33 AM.

Diff Detail

Event Timeline

milena.vujosevic.janicic retitled this revision from to Prevent usage of OR16_MMR6 instruction when code for microMIPS is generated. .
milena.vujosevic.janicic updated this object.
milena.vujosevic.janicic retitled this revision from Prevent usage of OR16_MMR6 instruction when code for microMIPS is generated. to [mips][microMIPS] Prevent usage of OR16_MMR6 instruction when code for microMIPS is generated. .Feb 18 2016, 2:34 AM
milena.vujosevic.janicic updated this object.
sdardis requested changes to this revision.Feb 18 2016, 4:50 AM
sdardis added a reviewer: sdardis.
sdardis edited edge metadata.
This revision now requires changes to proceed.Feb 18 2016, 4:50 AM

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 edited edge metadata.

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

dsanders added inline comments.Feb 19 2016, 3:22 AM
lib/Target/Mips/MicroMips32r6InstrInfo.td
814–815

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 edited edge metadata.

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 edited edge metadata.Feb 29 2016, 8:08 AM

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 edited edge metadata.

Simplified test case

dsanders accepted this revision.Mar 2 2016, 2:33 AM
dsanders edited edge metadata.

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).

This revision was automatically updated to reflect the committed changes.