This is an archive of the discontinued LLVM Phabricator instance.

[Flang][RISCV] Emit target features for RISC-V
ClosedPublic

Authored by sunshaoce on Mar 12 2023, 11:48 AM.

Details

Summary

Fix the issue of .o file generated by Flang
with Flags info is 0x0 under RISC-V.

Diff Detail

Event Timeline

sunshaoce created this revision.Mar 12 2023, 11:48 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 12 2023, 11:48 AM
sunshaoce requested review of this revision.Mar 12 2023, 11:48 AM

There is an existing test flang/test/Driver/target-cpu-features.f90 added in D137995

Would it be better to add RISCV to that file? or perhaps rename the tests to have target specific files?

clang/lib/Driver/ToolChains/Flang.cpp
114–116

identical code, could just do a fallthrough

jrtc27 added inline comments.Mar 12 2023, 1:39 PM
clang/lib/Driver/ToolChains/Flang.cpp
114–116

Why is this even conditional??

Why does "flang/test/Driver/target-features.f90" list all RISC-V features? Why not use https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/target-cpu-features.f90 instead?

flang/test/Driver/target-features.f90
1 ↗(On Diff #504460)

What happens if the RISC-V backend is not available?

4 ↗(On Diff #504460)

This one line will require updating once respective bit of LLVM is updated. Let's avoid that :)

jrtc27 added inline comments.Mar 13 2023, 1:20 AM
flang/test/Driver/target-features.f90
1 ↗(On Diff #504460)

Clang doesn't need a backend to be available to generate IR for that architecture. I would hope Flang is the same.

awarzynski added inline comments.Mar 13 2023, 1:26 AM
flang/test/Driver/target-features.f90
1 ↗(On Diff #504460)

Clang doesn't need a backend to be available to generate IR

This test is not generating LLVM IR. It does, however, specify the target which is then translated into many LLVM-specific flags.

Also:

clang --target=riscv64-linux-gnu --target=riscv64 -c file2.c
error: unable to create target: 'No available targets are compatible with triple "riscv64"'
1 error generated.
jrtc27 added inline comments.Mar 13 2023, 1:29 AM
flang/test/Driver/target-features.f90
1 ↗(On Diff #504460)

Because you're trying to use the backend. Pass --emit-llvm and observe it works.

sunshaoce updated this revision to Diff 504548.Mar 13 2023, 1:53 AM

Based on everyone's comments, I merged the test into target-cpu-features.f90 and only kept the generic -target-feature. At the same time, I removed the switch (TC.getArch()) and passed the test on x86 and RISC-V 64.

Fix the issue of .o file generated by Flang with Flags info is 0x0 under RISC-V.

TBH, I don't see how this is addressed in this patch. If that's something that this patch is intending to fix, then there should be a regression test added.

kiranchandramohan added inline comments.
clang/lib/Driver/ToolChains/Flang.cpp
114–116

There could be other ways to do this. But it provided a way to understand the list of supported architectures.

flang/test/Driver/target-cpu-features.f90
25–26

32-bit architectures are generally not supported. This test could be misleading.

awarzynski added inline comments.Mar 13 2023, 7:38 AM
flang/test/Driver/code-gen-rv64.f90
13

For those of us less familiar with RISC-V - could you explain what's significant about this line? For example, here it is made clear that with the right triple used, one should see a ret instruction within the main function (_QQmain). In here, I just see a "magic" number :)

sunshaoce updated this revision to Diff 504672.Mar 13 2023, 8:23 AM
sunshaoce marked an inline comment as done.

Address @awarzynski's comment. Thanks!

awarzynski added inline comments.Mar 13 2023, 8:33 AM
flang/test/Driver/code-gen-rv64.f90
2

Missing REQUIRES: - this test won't work unless the RISC-V backend is available.

13

Follow-up questions - what's "Flags" and why "0x5"? Is there any online documentation that you can refer to here?

sunshaoce updated this revision to Diff 504695.Mar 13 2023, 9:07 AM

Add REQUIRES in tests

flang/test/Driver/code-gen-rv64.f90
13
clementval resigned from this revision.Mar 13 2023, 9:09 AM
vzakhari added inline comments.
flang/test/Driver/target-cpu-features.f90
1

Can we split this test so that people who do not build riscv target still have aarch64/x86 targets tested in their workspaces?

awarzynski added inline comments.Mar 13 2023, 9:26 AM
flang/test/Driver/target-cpu-features.f90
1

Given that this test relies on -###, I think that REQUIRES can be safely removed (please double check)

sunshaoce marked 5 inline comments as done.Mar 13 2023, 9:53 AM
sunshaoce added inline comments.
flang/test/Driver/target-cpu-features.f90
33

Without REQUIRES line, test will be failed here.

Due to this patch has little to do with CPU-feature, I think we can only add code-gen-rv64.f90. How about that?

awarzynski added inline comments.Mar 13 2023, 10:22 AM
flang/test/Driver/code-gen-rv64.f90
13

Thanks - and could you add a note in the test that explains where the "magic" 0x5 comes from?

flang/test/Driver/target-cpu-features.f90
33

This line verifies the driver diagnostics, i.e. it's very different to what other lines test in this file. You can safely move it to a dedicated file, e.g. "target-cpu-features-invalid.f90".

sunshaoce marked 2 inline comments as done.

Add target-cpu-features-invalid.f90

jrtc27 added inline comments.Mar 13 2023, 11:04 AM
clang/lib/Driver/ToolChains/Flang.cpp
114–116

Well RISC-V needs -target-abi so its presence in the list does nothing to say it's properly supported. Nor do I think that's a good argument aside from that. You know every architecture needs to have its target features communicated, so making it conditional is not the right technical approach. Documenting the supported architectures should be done another way; perhaps in the actual documentation that developers/users are going to read, rather than buried in code?....

flang/test/Driver/code-gen-rv64.f90
8

Why do we need to go to an object file??? That's terrible practice in Clang tests, and the same should be true of Flang. Test the IR, that is sufficient, and decouples you from the backend.

flang/test/Driver/target-cpu-features-invalid.f90
13 ↗(On Diff #504756)

Don't these come from the backend? Testing them here doesn't seem right...

awarzynski added inline comments.Mar 13 2023, 11:31 AM
flang/test/Driver/code-gen-rv64.f90
8

That's terrible practice in Clang tests, and the same should be true of Flang. Test the IR, that is sufficient, and decouples you from the backend.

I disagree.

A compiler driver is responsible for creating a correct backend/LLVM invocation. Is there some other way to verify that the backend invocation is correct? (other then inspecting the generated machine code file).

I agree that it is desirable to avoid any architectural details leaking outside of LLVM (into e.g. Clang and/or Flang), but IMHO it's very hard to avoid in practice.

sunshaoce added inline comments.Mar 13 2023, 12:51 PM
flang/test/Driver/code-gen-rv64.f90
8

After my test, there is no difference between the LLVM IR generated before and after modification. These tests are also from or similar to those that already exist.

So I think we can land this patch first. Then fix these in a new patch.

awarzynski accepted this revision.Mar 13 2023, 1:04 PM

LGTM, thanks for contributing!

Please wait for the other reviewers to confirm that they are happy with these changes.

flang/test/Driver/target-cpu-features-invalid.f90
13 ↗(On Diff #504756)

Don't these come from the backend?

No. Both options are defined in Clang's Options.td:

This revision is now accepted and ready to land.Mar 13 2023, 1:04 PM
jrtc27 added inline comments.Mar 13 2023, 1:05 PM
flang/test/Driver/target-cpu-features-invalid.f90
13 ↗(On Diff #504756)

Then why do they need aarch64-registered-target? Either they're coming from the backend and so you need the backend, or they're coming from the frontend and so you don't need the backend.

awarzynski added inline comments.Mar 13 2023, 2:11 PM
flang/test/Driver/target-cpu-features-invalid.f90
13 ↗(On Diff #504756)

Can you clarify what you mean by "they"? I'm referring to driver's command line options, which is what this file is testing.

Either they're coming from the backend and so you need the backend, or they're coming from the frontend and so you don't need the backend.

Again, this is a driver test. The driver integrates the frontend and the backend (as well as other parts).

Are you suggesting that this test is moved to LLVM and a dependency on Flang is added to LLVM?

LG. Please wait for and OK from @jrtc27.

clang/lib/Driver/ToolChains/Flang.cpp
114–116

Putting the information in documentation also risks the fact that developers might not see it. Ideally, some portion of the compiler should provide an error if the target or a particular abi is not supported. I think we can discuss this specific issue in a separate patch.

Any other opinions? If not, I will land it later.

MaskRay added inline comments.Mar 14 2023, 2:47 PM
clang/lib/Driver/ToolChains/Flang.cpp
108–110

Remove [[fallthrough]] and just list the 3 case consecutively.

flang/test/Driver/target-cpu-features-invalid.f90
6 ↗(On Diff #504756)

! RUN: -o
For other subprojects, we prefer to indent continuation lines.

Address @MaskRay's comment

This revision was automatically updated to reflect the committed changes.
sunshaoce marked an inline comment as done.
mamrami added inline comments.
flang/test/Driver/code-gen-rv64.f90
2
awarzynski added inline comments.Mar 16 2023, 2:11 AM
flang/test/Driver/code-gen-rv64.f90
2

Sorry about that :(

line 3: llvm-readelf: command not found

There are 3 options:

HTH.

sunshaoce added inline comments.Mar 16 2023, 2:29 AM
flang/test/Driver/code-gen-rv64.f90
2

Thanks! I've submitted in https://reviews.llvm.org/D146204.

mamrami added inline comments.Mar 19 2023, 7:58 AM
flang/test/Driver/code-gen-rv64.f90
2
sunshaoce added inline comments.
flang/test/Driver/code-gen-rv64.f90
2

I noticed this error before, but @DavidSpickett haven't fixed yet. I'll fix it now.
https://reviews.llvm.org/rG9f93b71f20ea

mamrami added inline comments.Mar 19 2023, 8:22 AM
flang/test/Driver/code-gen-rv64.f90
2

thanks!