[PATCH] D29073: [ARM] Enable Cortex-M23 and Cortex-M33 support.

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

This patch has too many issues in one:

1. Adding new M cores to the target parser. The testing has to be done in the unit-test (as you've done). Check.

2. Adding new M cores to TableGen. Testing needs to be done by adding the new core to existing tests and using the appropriate CHECK lines (not adding new ones).

3. Adding a test to tail call, which should belong to https://reviews.llvm.org/D29020.

4. Heavily modifying the SW/HWDIV test, even though the hardware description you added in TableGen has no mention of special DIV features that would separate it from the other M cores.

I suggest you:

A. Keep the Target Parser and the TableGen changes here, but instead of modifying existing tests, just add new `RUN` lines to them where other M cores are tested and **reuse** the existing `CHECK` lines.

B. Move the tail-call test to https://reviews.llvm.org/D29020.

cheers,
--renato



================
Comment at: test/CodeGen/ARM/cortex-m23.ll:1
+; RUN: llc %s -o - -mtriple=thumbv8m.base -mcpu=cortex-m23 | FileCheck %s
+
----------------
This test is completely different to the patch, I'm not sure why you moved it here.


================
Comment at: test/CodeGen/ARM/div.ll:2
 ; RUN: llc < %s -mtriple=arm-apple-ios -mcpu=cortex-a8    | \
-; RUN:     FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-SWDIV
+; RUN:     FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-SWDIV -check-prefix=CHECK-SWDIV-64
 ; RUN: llc < %s -mtriple=arm-apple-ios -mcpu=swift        | \
----------------
Too many check lines increase the risk of tests being ignored, especially when you just rename them below, instead of adding new ones.


https://reviews.llvm.org/D29073