[PATCH] D88392: [M68k] (Patch 6/8) IR Tests

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

I'd prefer to see:

1. The ASM/ directory removed so Alloc/, Arith/ etc are directly under CodeGen/M68k, since those are the real CodeGen tests

2. the OBJ/ directory renamed to something like Encoding/ (the three-letter all-caps names are really ugly and not a style we tend to use), and the README updated to indicate that these are here solely because there is no integrated assembler support, and that the tests should be replaced by assembly->obj tests (which would live in llvm/test/MC/M68k) once that support exists.

3. I don't get what PAS stands for, but that directory should go. Just move the test wherever it belongs, presumably with the normal CodeGen tests (i.e. 1 above).

That should be a fairly easy set of `git mv`s.



================
Comment at: llvm/test/CodeGen/M68k/ASM/Arith/imul-neg.ll:4
+; FIXME: When using SelectionDAGISel, the following cases use
+; `sub` rather than the expected `neg`
+
----------------
RKSimon wrote:
> myhsu wrote:
> > RKSimon wrote:
> > > Please can you raise a bug for these?
> > Done: https://bugs.llvm.org/show_bug.cgi?id=49244
> Its up to you, but I'd probably prefer to run the script here to show the current codegen, keep the FIXME and drop the XFAIL - its a poor codegen issue not an actual error.
Agreed


================
Comment at: llvm/test/CodeGen/M68k/ASM/Arith/umul-with-overflow.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=m68k-linux-gnu -verify-machineinstrs| FileCheck %s
+
----------------



================
Comment at: llvm/test/CodeGen/M68k/ASM/CodeModel/medium-pie-global-access.ll:3
+; RUN: llc < %s -O2 -mtriple=m68k-linux-gnu -verify-machineinstrs \
+; RUN:              -code-model=medium -relocation-model=pic        \
+; RUN: | FileCheck %s
----------------
These backslashes don't line up; either line them up everywhere or only leave one space before the backslash. I count 71 cases of multiple spaces before a backslash; some of those might be lined up already but they should be audited. I'd be inclined to just do a mass `sed -i '/^[;#]/s/   *\\/ \\/` (and verify the result...) rather than going through and lining everything up (other backends don't bother).


================
Comment at: llvm/test/CodeGen/M68k/ASM/CodeModel/medium-pie-global-access.ll:4
+; RUN:              -code-model=medium -relocation-model=pic        \
+; RUN: | FileCheck %s
+
----------------
You have 7 cases of this; these should be indented, typically by an additional 2 (most common, ~5200 cases) or 4 spaces (less common, ~1700 cases)).


================
Comment at: llvm/test/CodeGen/M68k/OBJ/Arith/Classes/MxBiArOp_FMI.mir:3
+# RUN: llc %s -mtriple=m68k -start-after=prologepilog -O0 -filetype=obj -o - \
+# RUN:  | extract-section .text                                                \
+# RUN:  | FileCheck %s -check-prefixes=ADD8FI,ADD32FI,ADD8PI,ADD32PI,ADD8JI,ADD32JI
----------------
`RUN:  |` is wrong, should have either 1 or 3 more spaces (i.e. it's 2 or 4 more spaces of indentation plus the space between `:` and the shell command).


================
Comment at: llvm/test/CodeGen/M68k/OBJ/Arith/Classes/MxBiArOp_RFRM.mir:5
+# RUN:  | FileCheck %s -check-prefixes=ADD8DK,ADD32RK,ADD8DQ,ADD32RQ,ADD8DF,ADD32RF,\
+# RUN:ADD8DP,ADD32RP,ADD8DJ,ADD32RJ
+
----------------
You have loads of these badly-formatted cases


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

https://reviews.llvm.org/D88392