[PATCH] D43583: [MIPS GlobalISel] Adding GlobalISel

[Simon Dardis via Phabricator via llvm-commits]

sdardis added a comment.

LGTM, some nits inlined.

I'm also seeing -Wunused-private-field warnings for lines lib/Target/Mips/MipsInstructionSelector.cpp:32, 33, 36.

Not sure if you're seeing them.



================
Comment at: lib/Target/Mips/MipsCallLowering.cpp:1
+//===- llvm/lib/Target/MIPS/MIPSCallLowering.cpp - Call lowering-----------===//
+//
----------------
Missing C++ tag.


================
Comment at: lib/Target/Mips/MipsInstructionSelector.cpp:51
+  if (!isPreISelGenericOpcode(I.getOpcode())) {
+    // not global isel generic opcode
+    // TODO: select copy
----------------
Full sentence with full stop.


================
Comment at: lib/Target/Mips/MipsInstructionSelector.cpp:56
+
+  // we didn't select anything
+  return false;
----------------
Here too.


================
Comment at: test/CodeGen/Mips/GlobalISel/mips-irtranslator.ll:1
+; RUN: llc -march=mipsel -global-isel -stop-after=irtranslator -verify-machineinstrs %s -o - | FileCheck %s
+
----------------
This can go in test/CodeGen/Mips/GlobalISel/irtranslator/ret.ll


================
Comment at: test/CodeGen/Mips/GlobalISel/mips-isel.ll:1
+; RUN: llc -march=mipsel -global-isel -verify-machineinstrs %s -o - | FileCheck %s
+
----------------
Can you move this file to test/CodeGen/Mips/GlobalISel/llvm-ir/ret.ll

I'd like the test directory organised in the same way we do for normal instruction, just under GlobalISel/.

i. e. each particular construct gets it's own file, as our ISA variants and revisions can make the test files particularly big.

Can you also use -mtriple=mipsel-linux-gnu with the llc invocation, so that we can use update_llc_checks.py on these files. Also use the check prefix MIPS32.


================
Comment at: test/CodeGen/Mips/GlobalISel/mips-isel.ll:3
+
+define void @test_void_return() {
+; CHECK-LABEL: test_void_return:
----------------
You can just name the test functions that will go here after their types.


https://reviews.llvm.org/D43583