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

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham marked an inline comment as done.
simon_tatham 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
+
----------------
dmgreen wrote:
> 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 :) )
I think the shortcomings of that step-by-step testing strategy are exactly what caused the breakage in the first place.

If you know you're changing the mapping from driver arguments to cc1 feature options, then you change the test that //obviously// goes with the change you just made; but you leave unchanged the test that says the //previous// set of cc1 options caused the behaviour you wanted – because nothing will remind you that that needs changing as well (or make you aware that that test even exists to be updated, if you didn't already know).

An end-to-end test that ensures that //this// user input continues to cause //that// ultimate output code, regardless of the intervening details, would have prevented this bug arising last week, where the other strategy didn't. (Indeed, an end-to-end test of exactly that kind in our //downstream// tests is how we spotted it shortly after the change landed.)

So I still think I'd prefer not to remove this end-to-end test (though I will if people absolutely insist). But I'll add those extra checks you mention in `arm-mfpu.c`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62729