A pattern needed to match TruncIntFP was missing. This was causing multiple
tests from llvm test suite to fail during compilation for micromips.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
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. |
Comment Actions
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.
Comment Actions
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. |
| 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? |
Comment Actions
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. |