[PATCH] D62729: [ARM] Fix recent breakage of -mfpu=none.

Fri May 31 06:22:45 PDT 2019

dmgreen added inline comments.

================
Comment at: clang/test/CodeGen/arm-mfpu-none.c:2
+// REQUIRES: arm-registered-target
+// RUN: %clang -target arm-none-eabi -mcpu=cortex-m4 -mfpu=none -S -o - %s | FileCheck %s
+
----------------
simon_tatham wrote:
> dmgreen wrote:
> > lebedev.ri wrote:
> > > Generally clang codegen tests should test `-emit-llvm -S` output
> > Perhaps add to the tests in clang/test/Driver/arm-mfpu.c instead?
> @lebedev.ri : looking at the IR doesn't show up what I'm trying to test in this case. And I modelled this on existing tests in test/CodeGen, such as `arm-eabi.c`, which also need to test the real assembler output in order to do their job.
> 
> @dmgreen: it's true that I could also make `test/Driver/arm-mfpu.c` check that the extra feature name is appearing in the cc1 command line where I currently think it should be, but I think that wouldn't replace this test. The real point of this test is that the //currently// right feature names might not stay right, if the feature set is reorganised again in future – and if that happens, then whoever does it will probably rewrite most of `arm-mfpu.c` anyway.
> 
> So this is an end-to-end check that //whatever// detailed command line is generated by the driver in these circumstances, it is sufficient to ensure that the final generated code really doesn't include FP instructions.
I believe that the idea is that if we test each step, so make sure we have tests from clang driver -> clang-cl and clang -> llvm-ir and llvm -> asm, then it should all be tested. And because each has a well defined interfaces along each step of the way it should keep working. Any integration tests are often left to external tests like the test suite.

But I do see some precedent for this in other places. If you add the extra checks in test/Driver/arm-mfpu.c, then I think this is fine. (And I guess if someone objects we can always take it out :) )

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62729