Page MenuHomePhabricator

[mips] Introducing option -mabs=[legacy/2008]
ClosedPublic

Authored by abeserminji on Jul 28 2017, 2:00 AM.

Details

Summary

In patch D3274 using abs.[ds] instruction is forced, as they should behave in accordance with flags Has2008 and ABS2008. Unfortunately for revisions prior mips32r6 and mips64r6, abs.[ds] is not generating correct result when working with NaNs. To generate a sequence which always produce a correct result but also to allow user more control on how his code is compiled, option -mabs is added where user can choose legacy or 2008. By default legacy mode is used on revisions prior R6. Mips32r6 and mips64r6 use abs2008 mode by default.

Part of this patch is taken from patch D3274.

This is LLVM part of patch. Clang part can be found here: https://reviews.llvm.org/D35982

Diff Detail

Repository
rL LLVM

Event Timeline

abeserminji created this revision.Jul 28 2017, 2:00 AM
abeserminji edited the summary of this revision. (Show Details)Jul 28 2017, 2:01 AM
sdardis added inline comments.Aug 11 2017, 4:48 AM
CodeGen/Mips/fabs.ll
3 ↗(On Diff #108597)

abs.[ds] does not generate the correct result when working with NaNs

4 ↗(On Diff #108597)

enabled

5 ↗(On Diff #108597)

Also, restore the comment that begins with "In 1985 mode," to give better context as to what's being tested.

6–26 ↗(On Diff #108597)

This also requires some big endian tests.

Target/Mips/Mips.td
83 ↗(On Diff #108597)

Indentation is wrong here, this line should be aligned to the first quotation mark of "abs2008".

Target/Mips/MipsISelLowering.cpp
335 ↗(On Diff #108597)

Demorgan's law: change !A && !B to !(A || B), it's somewhat easier to immediately see it is correct.

2428 ↗(On Diff #108597)

hasMips64() there should be checking if the ABI is N32 or N64.

Target/Mips/MipsInstrInfo.td
226 ↗(On Diff #108597)

Overly long line. You can break it like so:

def UseAbs :          Predicate<"Subtarget->inAbs2008Mode() ||"
                                "TM.Options.NoNaNsFPMath">;
Target/Mips/MipsSubtarget.cpp
106–107 ↗(On Diff #108597)

These if statements can be joined together.

108 ↗(On Diff #108597)

This line is not clang-formatted.

sdardis requested changes to this revision.Aug 11 2017, 4:48 AM
This revision now requires changes to proceed.Aug 11 2017, 4:48 AM
abeserminji marked 8 inline comments as done.Aug 16 2017, 6:28 AM
abeserminji edited edge metadata.
abeserminji marked 2 inline comments as done.

Resolved all the comments.

You'll also need to guard the microMIPSr3 definitions of fabs with the UseAbs and extend the fabs test case for microMIPS.

lib/Target/Mips/MipsISelLowering.cpp
2426–2427 ↗(On Diff #111499)

For MIPS cpus that lack ins / ext instructions, DAGCombine performs this transformation: (srl (shl x, c), c) -> (and x, cst2).

So some follow up work would be to provide DAG patterns which transform (and x, cst) back to a (shl (srl x (clz cst)) (clz cst)) sequence.

I do have a stale patch which addresses this.

2433–2436 ↗(On Diff #111499)

FIXME: For mips32r2, the sequence of (BuildPairF64 (ins (ExtractElementF64 Op 1), $zero, 31 1) (ExtractElementF64 Op 0)) and the Op has one use, we should be able to drop the usage of mfc1/mtc1 and rewrite the register in place.

lib/Target/Mips/MipsInstrFPU.td
428 ↗(On Diff #111499)

This requires NotInMicroMips as well.

test/CodeGen/Mips/fabs.ll
25–26 ↗(On Diff #111499)

I believe this should be -mcpu=mips64r2 (and correct the triple to mips64).

47–48 ↗(On Diff #111499)

This I believe should be -mcpu=mips64r2 (and correct the triple to mips64).

sdardis requested changes to this revision.Aug 22 2017, 5:34 AM
This revision now requires changes to proceed.Aug 22 2017, 5:34 AM
abeserminji added inline comments.Aug 23 2017, 6:48 AM
lib/Target/Mips/MipsISelLowering.cpp
2426–2427 ↗(On Diff #111499)

Is it ok if I add a TODO comment here, so you can submit you patch later?

2433–2436 ↗(On Diff #111499)

Should I just add this as a comment above the return statement?

sdardis added inline comments.Aug 23 2017, 7:05 AM
lib/Target/Mips/MipsISelLowering.cpp
2426–2427 ↗(On Diff #111499)

Yes, that's fine.

2433–2436 ↗(On Diff #111499)

Just above the 'SDValue LowX'.

abeserminji edited edge metadata.
abeserminji marked 9 inline comments as done.

Comments resolved:

  • Added new run checks for microMIPS
  • Corrected existing run check lines
  • Added NotInMicroMips to the FABS guard in Mips
  • Guarded FABS in microMIPS with UseAbs
  • Added comments for future work
abeserminji added inline comments.Aug 24 2017, 6:07 AM
test/CodeGen/Mips/fabs.ll
54 ↗(On Diff #112538)

I was unable to test microMIPS for some CPUs (mips64, mips32r6, ...) due errors like:

  • LLVM ERROR: ran out of registers during register allocation
  • LLVM ERROR: Cannot select: t11: f64 = bitcast t14

Do you have any recommendation what to do about it?

sdardis added inline comments.Aug 24 2017, 6:13 AM
test/CodeGen/Mips/fabs.ll
54 ↗(On Diff #112538)

Can you run it with -debug and check what registers classes are in use? If you're seeing AFGR64 and FGR64 at the same time, you've hit the bug I suspected.

That bug is where microMIPS AFGR64 using instructions are also available with FGR64 instructions, except that the entire AFGR register set is reserved. The register allocator then errors out because ISel has "incorrectly" selected the AFGR instructions.

abeserminji added inline comments.Aug 24 2017, 7:30 AM
test/CodeGen/Mips/fabs.ll
54 ↗(On Diff #112538)

Yes, that's exactly what I get. FGR64 and AFGR64 registers are in use at the same time.

sdardis added inline comments.Aug 24 2017, 8:33 AM
test/CodeGen/Mips/fabs.ll
54 ↗(On Diff #112538)

I had hoped to defer that work for a bit. In summary, we need to fix the predicates that guard the micromips instruction set, as currently it's somewhat broken.

The first step towards fixing that issue is to change the overall Predicate = [InMicroMips] for the microMIPS FPU instructions to be AdditionalPredicates = [InMicroMips], then annotate all the floating point instructions there with the correct predicates (ISA, FPU model etc).

The next step is to add predicates for micromips encoding, mips16 encoding, rework the instruction classes not set the encoding predicate, and having a joint encoding + ISA level predicate determining if an instruction is available for a given ISA level and encoding, rather than separating it out into AdditionalPredicates.

Can you take a look at the first step?

Thanks,
Simon

sdardis accepted this revision.Oct 11 2017, 3:31 AM

LGTM as rL315362 should fix the register allocation issue.

lib/Target/Mips/Mips.td
83 ↗(On Diff #112538)

abs -> abs.fmt mode

lib/Target/Mips/MipsSubtarget.cpp
109 ↗(On Diff #112538)

Abs -> abs.fmt

lib/Target/Mips/MipsSubtarget.h
81 ↗(On Diff #112538)

Abs -> abs.fmt

This revision is now accepted and ready to land.Oct 11 2017, 3:31 AM
abeserminji marked 3 inline comments as done.Oct 12 2017, 6:17 AM

Now I get an error for abs.[sd] in this test test/MC/Mips/micromips-fpu-instructions.s, saying that instruction requires a CPU feature not currently enabled. I checked the instruction definitions, and one solution to avoid the error would be to remove NotInMicroMips from AdditionalPredicates at 428 in MipsInstrFPU.td. As abs.fmt instruction in MipsInstrFPU.td has MMRel, my guess would be that NotInMicroMips should not be in AdditionalPredicates. Or is there other way to solve this?

The root of the problem is that many of the microMIPS FPU instructions are marked as isCodeGenOnly, which means that they aren't in the assembler matching table or disassembler table. This means that although the microMIPS32R6 definitions are used, we can't match them as those definitions have differing feature bits to what the invocation of llvm-mc has in that test. I've got a WIP patch to being addressing this. Hold off on trying to get this patch in for the moment.

+CCing the llvm-commits.

abeserminji marked 4 inline comments as done.
abeserminji added reviewers: atanasyan, smaksimovic.

Rebased and updated the patch. Now it seems like the past issues are gone.

This revision was automatically updated to reflect the committed changes.