This is an archive of the discontinued LLVM Phabricator instance.

[MIPS] [microMIPS] Pattern match TruncIntFP
ClosedPublic

Authored by mbrkusanin on Feb 27 2019, 9:07 AM.

Details

Summary

A pattern needed to match TruncIntFP was missing. This was causing multiple
tests from llvm test suite to fail during compilation for micromips.

Diff Detail

Repository
rL LLVM

Event Timeline

mbrkusanin created this revision.Feb 27 2019, 9:07 AM
sdardis accepted this revision.Feb 28 2019, 3:48 PM

Two comments, and some more inlined. Check the formatting of the commit message
body, it should be 80 cols max. Otherwise the log messages can look strange due
to wrapping (I'm using git).

Second, after committing this, add more RUN: lines to this test to cover the
likes of MIPS32, MIPS32r2 with and without FP64, with soft-float, MIPS-III, MIPS64,
and MIPSR6 along with microMIPSR6 to document current behaviour.

Otherwise, LGTM with my comments addressed without the test additions for other
FPU configurations.

test/CodeGen/Mips/trunc-w-s-mm.ll
1 ↗(On Diff #188563)

This file should be in test/CodeGen/Mips/llvm-ir/fptosi.ll - that test directory is for testing the matching of LLVM IR constructs through ISel to machine code.

2 ↗(On Diff #188563)

-relocation-model=pic is not needed for this test.

4 ↗(On Diff #188563)

Small nit: For the test/CodeGen/Mips/llvm-ir/ directory, the comment describing the purpose of the test should be along the lines of 'Test that fptosi can be matched for MIPS targets for various FPU configurations' .

18–20 ↗(On Diff #188563)

Cut these three lines from the test and perform fptosi directly on the float parameter. The alloca, store, load IR constructs are uninteresting from a testing perspective.

This revision is now accepted and ready to land.Feb 28 2019, 3:48 PM
mbrkusanin updated this revision to Diff 189508.Mar 6 2019, 7:23 AM
mbrkusanin edited the summary of this revision. (Show Details)

Pattern matches for other configurations are added. Thanks for mentioning that. I've also added -asm-show-inst to RUN: lines that show which version of TRUNC_W will be chosen since that is what this patch is doing.

mbrkusanin retitled this revision from [MIPS] [microMIPS] Pattern match TRUNC_W_S_MM to [MIPS] [microMIPS] Pattern match TruncIntFP.Mar 6 2019, 7:24 AM
mbrkusanin marked 4 inline comments as done.
sdardis requested changes to this revision.Mar 6 2019, 11:33 AM

Some minor nits inlined, but otherwise the patch is more or less there.

lib/Target/Mips/MicroMipsInstrFPU.td
433–435 ↗(On Diff #189508)

This pattern is duplicated by the one above except for the predicates. Drop the 'FGR_32' in the pattern above and remove this pattern. For pure FGR32 operand using instructions, they shouldn't need to distinguish between the 32bit or 64bit FPU configurations.

test/CodeGen/Mips/llvm-ir/fptosi.ll
2 ↗(On Diff #189508)

Break the line with '\' after the '|', and only include only one prefix with -check-prefix. Just make sure it's unique in the tests. Drop the excessive whitespace between the arguments.

13 ↗(On Diff #189508)

You should also test microMIPS softfloat for completeness sake.

15 ↗(On Diff #189508)

This test is unnecessary as microMIPS32R6 requires a full 64-bit fpu. See Introduction to the microMIPS32 Architecture v6.0, Chapter 7, binary compatibility, page 103.

This revision now requires changes to proceed.Mar 6 2019, 11:33 AM
mbrkusanin updated this revision to Diff 189705.Mar 7 2019, 9:03 AM
mbrkusanin marked 4 inline comments as done.
mbrkusanin marked an inline comment as done.Mar 7 2019, 9:08 AM
mbrkusanin added inline comments.
test/CodeGen/Mips/llvm-ir/fptosi.ll
2 ↗(On Diff #189508)

Can you just clarify what you meant by 'unique'. Unique per RUN: line or unique per generated output with utils/update_llc_test_checks.py?
Right now it is changed to latter (first two RUN:s have same prefix 'M32' since they generate the same output).

mbrkusanin marked an inline comment as not done.Mar 7 2019, 9:11 AM
mbrkusanin marked an inline comment as not done.
sdardis accepted this revision.Mar 7 2019, 11:44 AM
sdardis marked an inline comment as done.

One nit inline, but otherwise LGTM.

test/CodeGen/Mips/llvm-ir/fptosi.ll
30 ↗(On Diff #189705)

The ',fp64' isn't required here as microMIPSR6 requires a 64bit FPU.

2 ↗(On Diff #189508)

I would agree with the latter interpretation (unique per output) in this case. Sorry for the ambivalence. There can be cases where mips32 and mips32r2 for example generate different output. In this case, as they generate the same output it's fine to merge the the check prefixes. utils/update_llc_test_checks.py does complain when multiple RUN: lines have the same check prefixes but different output.

This revision is now accepted and ready to land.Mar 7 2019, 11:44 AM
mbrkusanin updated this revision to Diff 189841.Mar 8 2019, 4:07 AM
mbrkusanin marked 3 inline comments as done.

Addressed review comments.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2019, 7:12 AM
Herald added a subscriber: jrtc27. · View Herald Transcript