This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen][RISCV] Fix clang/test/CodeGen/atomic_ops.c for RISC-V
ClosedPublic

Authored by luismarques on Feb 19 2020, 9:27 AM.

Details

Summary

By default the RISC-V target doesn't have the atomics standard extension enabled. The first RUN line in clang/test/CodeGen/atomic_ops.c doesn't specify a target triple, which means that on RISC-V Linux hosts it will target RISC-V, but because we use clang cc1 we don't get the toolchain driver functionality to automatically turn on the extensions implied by the target triple (riscv64-linux includes atomics). This causes the test to fail on RISC-V hosts.

This patch changes the test to have RUN lines for two targets, one with native atomics and one without. To work around FileCheck limitations and more accurately match the output, some tests now have separate prefixes for the two cases.

Diff Detail

Event Timeline

luismarques created this revision.Feb 19 2020, 9:27 AM

I'm not really a big fan of running tests with the host target triple, anyway; it seems to create work with almost no benefit. I'd be happy to just run the test with one target that has native atomics, and one target that doesn't. (The relevant code is all target-independent, aside from the atomic widths, so we aren't really gaining anything by testing more targets.)

On a side-note, we could enhance the test runner to support "XFAIL: riscv32-default-target" if we thought it would be useful. But again, I'm not a fan of tests that depend on the default target in the first place.

I'm not really a big fan of running tests with the host target triple, anyway; it seems to create work with almost no benefit. I'd be happy to just run the test with one target that has native atomics, and one target that doesn't. (The relevant code is all target-independent, aside from the atomic widths, so we aren't really gaining anything by testing more targets.)

That sounds fine to me. I might need to rewrite the tests with two different prefixes though (e.g. NATIVE- vs LIBCALL-), since I can't see how to mix substitution blocks with regex alternatives. For instance, we have this:

// CHECK: %[[load:.*]] = {{load atomic i8, i8\* @b seq_cst|}}

You need to add an alternative for an __atomic_load libcall instead of the load atomic, but then the "assignment" to %atomic-temp moves to the right, as an out argument, so you'd need the variable capture to move to inside the regex, which I don't think is supported.
Is that OK, using different prefixes? Arguably that's a better test anyway, since you can then check your expectation that certain targets triples match certain alternative matches.

Yes, makes sense

luismarques edited the summary of this revision. (Show Details)

As suggested by @efriedma, the patch was reworked to have one target with native atomics, and one without. No RUN run with a default target remains.

This revision is now accepted and ready to land.Feb 21 2020, 10:14 AM
This revision was automatically updated to reflect the committed changes.