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

[sunshaoce]

Summary

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

[mnadeem]

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]

clang/lib/Driver/ToolChains/Flang.cpp

114–116

Why is this even conditional??

[awarzynski]

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]

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]

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]

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]

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.

[awarzynski]

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]

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.

[sunshaoce]

Add test

[awarzynski]

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]

Address @awarzynski's comment. Thanks!

[awarzynski]

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]

Add REQUIRES in tests

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

13

You can refer: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/download/v1.0/riscv-abi.pdf 8.1. File Header e_flags section

[vzakhari]

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]

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]

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]

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]

Add target-cpu-features-invalid.f90

[jrtc27]

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]

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]

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]

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:

• -target-feaure
• -target-cpu

[jrtc27]

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]

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?

[kiranchandramohan]

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.

[sunshaoce]

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

[MaskRay]

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.

[sunshaoce]

Address @MaskRay's comment

[mamrami]

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

2

Hi :)
It seems like the test fails: https://buildkite.com/llvm-project/premerge-checks/builds/141302#0186e55e-199e-401d-ab9f-9f3d47ec87af
I see it in my non related patch https://reviews.llvm.org/D146132

[awarzynski]

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

2

Sorry about that :(

line 3: llvm-readelf: command not found

There are 3 options:

• revert
• add llvm-readelf here https://github.com/llvm/llvm-project/blob/main/flang/test/CMakeLists.txt#L61
• updated the test to use llvm-objdump instead

HTH.

[sunshaoce]

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

2

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

[mamrami]

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

2

Hi again!
The test passed after the fix but it fails again:
https://buildkite.com/llvm-project/premerge-checks/builds/141905#0186fa14-2339-4c0c-9966-dea23592a86b

[sunshaoce]

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]

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

2

thanks!

clang/lib/Driver/ToolChains/Flang.cpp

if (!CPU.empty()) {
     CmdArgs.push_back(Args.MakeArgString(CPU));
   }
 
   // Add the target features.
   switch (TC.getArch()) {
   default:
     break;
   case llvm::Triple::aarch64:
     [[fallthrough]];
+  case llvm::Triple::riscv64:
+    [[fallthrough]];
   case llvm::Triple::x86_64:
     getTargetFeatures(D, Triple, Args, CmdArgs, /*ForAs*/ false);
     break;
   }
 
   // TODO: Add target specific flags, ABI, mtune option etc.
 }
 
 static void addFloatingPointOptions(const Driver &D, const ArgList &Args,
                                     ArgStringList &CmdArgs) {
   StringRef FPContract;
   bool HonorINFs = true;
   bool HonorNaNs = true;
   bool ApproxFunc = false;

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

+! Test -emit-obj (RISC-V 64)
+
+! RUN: rm -f %t.o
+! RUN: %flang_fc1 -triple riscv64-unknown-linux-gnu \
+! RUN:   -target-feature +d -target-feature +c -emit-obj %s -o %t.o
+! RUN: llvm-readelf -h %t.o | FileCheck %s
+
+! RUN: rm -f %t.o
+! RUN: %flang --target=riscv64-unknown-linux-gnu -c %s -o %t.o
+! RUN: llvm-readelf -h %t.o | FileCheck %s
+
+! If Flang failed to emit target-feature info, then Flags will be 0x0.
+! CHECK: Flags: 0x5, RVC, double-float ABI
+end program

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

 ! REQUIRES: aarch64-registered-target, x86-registered-target
 
 ! Test that -mcpu/march are used and that the -target-cpu and -target-features
 ! are also added to the fc1 command.
 
 ! RUN: %flang --target=aarch64-linux-gnu -mcpu=cortex-a57 -c %s -### 2>&1 \
 ! RUN: | FileCheck %s -check-prefix=CHECK-A57
 
 ! RUN: %flang --target=aarch64-linux-gnu -mcpu=cortex-a76 -c %s -### 2>&1 \
 ! RUN: | FileCheck %s -check-prefix=CHECK-A76
 
 ! RUN: %flang --target=aarch64-linux-gnu -march=armv9 -c %s -### 2>&1 \
 ! RUN: | FileCheck %s -check-prefix=CHECK-ARMV9
 
 ! Negative test. ARM cpu with x86 target.
 ! RUN: %flang --target=x86_64-linux-gnu -mcpu=cortex-a57 -c %s -### 2>&1 \
 ! RUN: | FileCheck %s -check-prefix=CHECK-NO-A57
 
 ! RUN: %flang --target=x86_64-linux-gnu -march=skylake -c %s -### 2>&1 \
 ! RUN: | FileCheck %s -check-prefix=CHECK-SKYLAKE
 
 ! RUN: %flang --target=x86_64h-linux-gnu -c %s -### 2>&1 \
 ! RUN: | FileCheck %s -check-prefix=CHECK-X86_64H
 
+! RUN: %flang --target=riscv64-linux-gnu -c %s -### 2>&1 \
+! RUN: | FileCheck %s -check-prefix=CHECK-RV64
+
 
 ! Test that invalid cpu and features are ignored.
 
 ! RUN: %flang_fc1 -triple aarch64-linux-gnu -target-cpu supercpu \
 ! RUN: -o /dev/null -S %s 2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-CPU
 
 ! RUN: %flang_fc1 -triple aarch64-linux-gnu -target-feature +superspeed \
 ! RUN: -o /dev/null -S %s 2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-FEATURE
 
 
 ! CHECK-A57: "-fc1" "-triple" "aarch64-unknown-linux-gnu"
 ! CHECK-A57-SAME: "-target-cpu" "cortex-a57" "-target-feature" "+v8a" "-target-feature" "+aes" "-target-feature" "+crc" "-target-feature" "+fp-armv8" "-target-feature" "+sha2" "-target-feature" "+neon"
 
 ! CHECK-A76: "-fc1" "-triple" "aarch64-unknown-linux-gnu"
 ! CHECK-A76-SAME: "-target-cpu" "cortex-a76" "-target-feature" "+v8.2a" "-target-feature" "+aes" "-target-feature" "+crc" "-target-feature" "+dotprod" "-target-feature" "+fp-armv8" "-target-feature" "+fullfp16" "-target-feature" "+lse" "-target-feature" "+ras" "-target-feature" "+rcpc" "-target-feature" "+rdm" "-target-feature" "+sha2" "-target-feature" "+neon" "-target-feature" "+ssbs"
 
 ! CHECK-ARMV9: "-fc1" "-triple" "aarch64-unknown-linux-gnu"
 ! CHECK-ARMV9-SAME: "-target-cpu" "generic" "-target-feature" "+neon" "-target-feature" "+v9a" "-target-feature" "+sve" "-target-feature" "+sve2"
 
 ! CHECK-NO-A57: "-fc1" "-triple" "x86_64-unknown-linux-gnu"
 ! CHECK-NO-A57-NOT: cortex-a57
 ! CHECK-NO-A57-SAME: "-target-cpu" "x86-64"
 ! CHECK-NO-A57-NOT: cortex-a57
 
 ! CHECK-SKYLAKE: "-fc1" "-triple" "x86_64-unknown-linux-gnu"
 ! CHECK-SKYLAKE-SAME: "-target-cpu" "skylake"
 
 ! CHECK-X86_64H: "-fc1" "-triple" "x86_64h-unknown-linux-gnu"
 ! CHECK-X86_64H-SAME: "-target-cpu" "x86-64" "-target-feature" "-rdrnd" "-target-feature" "-aes" "-target-feature" "-pclmul" "-target-feature" "-rtm" "-target-feature" "-fsgsbase"
 
+! CHECK-RV64: "-fc1" "-triple" "riscv64-unknown-linux-gnu"
+! CHECK-RV64-SAME: "-target-cpu" "generic-rv64" "-target-feature" "+m" "-target-feature" "+a" "-target-feature" "+f" "-target-feature" "+d" "-target-feature" "+c"
+
+
 ! CHECK-INVALID-CPU: 'supercpu' is not a recognized processor for this target (ignoring processor)
 ! CHECK-INVALID-FEATURE: '+superspeed' is not a recognized feature for this target (ignoring feature)