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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
