Page MenuHomePhabricator

[Flang][Driver] Handle target CPU and features
ClosedPublic

Authored by mnadeem on Nov 14 2022, 5:07 PM.

Details

Summary

This patch:

  • Adds target-feature and target-cpu to FC1Options.
  • Moves getTargetFeatures() from Clang.cpp to CommonArgs.cpp.
  • Processes target cpu and features in the flang driver. Right now features are only added for AArch64/x86 because I only did basic testing on them but it should generally work for others as well. Option handling is similar to clang.
  • Adds appropriate structures in TargetOptions and passes them to the target machine.

What's missing:

  • Adding the CPU info and the features as attributes in the LLVM IR module.
  • Processing target specific flags, e.g. SVE vector bits for AArch64, ABI etc.

Diff Detail

Event Timeline

mnadeem created this revision.Nov 14 2022, 5:07 PM
Herald added a project: Restricted Project. · View Herald Transcript
mnadeem requested review of this revision.Nov 14 2022, 5:07 PM

Thanks @mnadeem for this patch. A few minor comments first. Try to replace auto in all places except where the type is on the RHS.

We might need -fc1 tests as well.

clang/lib/Driver/ToolChains/Flang.cpp
85

Nit: leftover code?

86

Nit: replace auto.

99

I get a segfault in Fortran::frontend::CodeGenAction::setUpTargetMachine() currently when using ./bin/flang-new --target=x86_64-linux-gnu file.f90

flang/lib/Frontend/CompilerInvocation.cpp
179

Nit: Can you remove the auto here?

flang/lib/Frontend/FrontendActions.cpp
594–595

Nit: Can you remove the auto here?

Matt added a subscriber: Matt.Nov 21 2022, 3:12 PM

Thanks for implementing this!

Processes target cpu and features in the flang driver. Right now features are only added for AArch64 because I only did basic testing on AArch64 but it should generally work for others as well.

X86 is a very popular target and we have pre-commit CI as well. And X86 buildbots :) Please include X86.

Question: are the option semantics identical that what you get in clang -cc1? If yes, could you add a comment in the summary?

mnadeem updated this revision to Diff 478766.Nov 29 2022, 5:38 PM
mnadeem marked 4 inline comments as done.
mnadeem edited the summary of this revision. (Show Details)

Thanks for implementing this!

Processes target cpu and features in the flang driver. Right now features are only added for AArch64 because I only did basic testing on AArch64 but it should generally work for others as well.

X86 is a very popular target and we have pre-commit CI as well. And X86 buildbots :) Please include X86.

Question: are the option semantics identical that what you get in clang -cc1? If yes, could you add a comment in the summary?

Included x86 and added a comment in the summary.

We might need -fc1 tests as well.

What kind of tests do you think would be appropriate here? Can you point me to any examples, maybe from clang?

mnadeem added inline comments.Nov 29 2022, 5:44 PM
clang/lib/Driver/ToolChains/Flang.cpp
99

I am unable to reproduce this, do you have your toolchain built for x86 target as well?

mnadeem added inline comments.Nov 29 2022, 5:45 PM
clang/lib/Driver/ToolChains/Flang.cpp
99

It might be unrelated to this patch.

Thanks for all the updates @mnadeem! Mostly looks good. A few more nits, but nothing substantial :)

We might need -fc1 tests as well.

What kind of tests do you think would be appropriate here? Can you point me to any examples, maybe from clang?

I didn't see any tests in Clang specifically for the frontend driver flags taht are added here (i.e. -target-cpu` and -target-feature). However, you could add a test for situations like this: -target-cpu=my-imaginary-cpu and -target-fature=my-amazing-non-existent-feature.

clang/lib/Driver/ToolChains/Flang.cpp
98

[nit] I don't think that this comment contributes much. To me it is self-explanatory that only the triples that are actually present here are tested. Having said that, I don't mind keeping it here.

100

nit

flang/include/flang/Frontend/TargetOptions.h
24–25

I think that we can trim this now. WDYT?

flang/test/Driver/target-cpu-features.f90
26–27
  1. By adding -fc1 you make it clear that it's the frontend driver invocation that's being tested.
  2. CHECK-SAME makes sure the triple is matched with the right set of features.
mnadeem updated this revision to Diff 479474.Dec 1 2022, 4:38 PM
mnadeem marked 4 inline comments as done.
  • Address comments.
  • Add fc1 tests.
awarzynski accepted this revision.Dec 2 2022, 12:07 AM

LGTM, thank you!

This revision is now accepted and ready to land.Dec 2 2022, 12:07 AM
This revision was automatically updated to reflect the committed changes.