[PATCH] D58722: [MIPS] [microMIPS] Pattern match TRUNC_W_S_MM

[Simon Dardis via Phabricator via llvm-commits]

sdardis accepted this revision.
sdardis added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: test/CodeGen/Mips/trunc-w-s-mm.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=mipsel-linux-gnu -mattr=+micromips -relocation-model=pic < %s | FileCheck %s
----------------
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.


================
Comment at: test/CodeGen/Mips/trunc-w-s-mm.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=mipsel-linux-gnu -mattr=+micromips -relocation-model=pic < %s | FileCheck %s
+
----------------
-relocation-model=pic is not needed for this test.


================
Comment at: test/CodeGen/Mips/trunc-w-s-mm.ll:4
+
+; Test that should fail if there is no DAGISel pattern for TruncIntFP to
+; to TRUNC_W_S_MM
----------------
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' . 


================
Comment at: test/CodeGen/Mips/trunc-w-s-mm.ll:18-20
+  %t.addr = alloca float, align 4
+  store float %t, float* %t.addr, align 4
+  %0 = load float, float* %t.addr, align 4
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58722/new/

https://reviews.llvm.org/D58722