This is an archive of the discontinued LLVM Phabricator instance.

[Clang][Flang][AMDGPU] Add support for AMDGPU to Flang driver
ClosedPublic

Authored by domada on Mar 8 2023, 4:57 AM.

Details

Summary

Scope of changes:

  1. Extract common code between Clang and Flang for parsing AMDGPU features
  2. Add function which adds implicit target features for AMDGPU as Clang does
  3. Add AMDGPU target as one of valid targets for Flang

Diff Detail

Event Timeline

domada created this revision.Mar 8 2023, 4:57 AM
Herald added a project: Restricted Project. · View Herald Transcript
domada requested review of this revision.Mar 8 2023, 4:57 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
domada updated this revision to Diff 503672.Mar 9 2023, 1:02 AM

Fixed formatting

domada retitled this revision from [Flang][AMDGPU][OpenMP] Save target features in OpenMP MLIR dialect to [WIP][Flang][AMDGPU][OpenMP] Save target features in OpenMP MLIR dialect.Mar 16 2023, 5:41 AM
domada updated this revision to Diff 507269.Mar 22 2023, 2:09 AM
domada retitled this revision from [WIP][Flang][AMDGPU][OpenMP] Save target features in OpenMP MLIR dialect to [Flang][AMDGPU][OpenMP] Save target features in OpenMP MLIR dialect.

Patch rebased

D146612 presents the lowering from MLIR attributes to LLVM IR.

Really nice to see some shared code being elevated out of Clang into LLVM, thanks!

I've only reviewed on the Flang driver changes. I will let the OpenMP experts to review the remaining bits. All in all looks good, I've only made some small suggestions.

Thanks for working on this!

flang/lib/Frontend/FrontendActions.cpp
93

This method could be simplified by following https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code. For example:

std::string CodeGenAction::getAllTargetFeatures()  {
  if (!triple.isAMDGPU()) {
    allFeaturesStr = llvm::join(targetOpts.featuresAsWritten.begin(),
                                targetOpts.featuresAsWritten.end(), ",");
    return allFeaturesStr;
  }

  // The logic for AMDGPU
  // Perhaps add a dedicated hook: getExplicitAndImplicitAMDGPUTargetFeatures()
}

Btw, this method does different things depending on the triple. Perhaps rename it as something more generic, e.g. getTargetFeatures? I think that the current name, getAllTargetFeatures, is a bit misleading (i.e. what does "all" mean?).

Also:

  • make it static
  • document
domada updated this revision to Diff 507333.Mar 22 2023, 6:18 AM
domada retitled this revision from [Flang][AMDGPU][OpenMP] Save target features in OpenMP MLIR dialect to [Flang][AMDGPU] Add support for AMDGPU to Flang driver.
domada edited the summary of this revision. (Show Details)

Applied remarks.
Moved OpenMP changes to https://reviews.llvm.org/D146612 .

domada marked an inline comment as done.Mar 22 2023, 6:29 AM
domada added inline comments.
flang/lib/Frontend/FrontendActions.cpp
93

Hi,
thanks for the review. I applied your remarks. I also moved OpenMP related changes to the child review.

A few more comments, but mostly nits. Btw, is this patch sufficient to generate code for AMDGPU? Or, put differently, what's the level of support atm?

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

Should there be a test for this triple as well?

flang/lib/Frontend/FrontendActions.cpp
134–137

[nit] I would probably flip this as:

if (triple.isAMDGPU()) {
  // Clang does not append all target features to the clang -cc1 invocation.
  // Some AMDGPU features are passed implicitly by the Clang frontend.
  // That's why we need to extract implicit AMDGPU target features and add
  // them to the target features specified by the user
  return getExplicitAndImplicitAMDGPUTargetFeatures(ci, targetOpts, triple);
  }

I know that I suggested the opposite in my previous comment, but you have simplified this since :) In any case, feel free to ignore.

139–142

[nit] IMHO this is documenting an implementation detail that would be more relevant inside getExplicitAndImplicitAMDGPUTargetFeatures.

More importantly, you are suggesting that the driver is doing whatever it is doing "because that's what Clang does". Consistency with Clang is important (you could call it out in the commit message) :) However, it would be even nicer to understand the actual rationale behind these implicit features. Any idea? Perhaps there are some clues in git history?

Also, perhaps a TODO to share this code with Clang? (to make it even clearer that the frontend driver aims for full consistency with Clang here).

domada marked an inline comment as done.Mar 23 2023, 4:25 AM
domada added inline comments.
flang/lib/Frontend/FrontendActions.cpp
139–142

I think that the main difference between Clang and Flang is the lack of TargetInfo class.

TargetInfo classes are Clang specific and they are responsible for parsing/adding default target features. Every target performs initialization in different way (compare for example AArch64 vs AMDGPU target initialization.

I don't want to make TargetInfo class Clang indendent (see discussion: https://discourse.llvm.org/t/rfc-targetinfo-library/64342 ). I also don't want to reimplement the whole TargetInfo class in Flang, because Flang already uses LLVM TargetMachine class (see: https://llvm.org/doxygen/classllvm_1_1TargetMachine.html and https://github.com/llvm/llvm-project/blob/main/flang/lib/Frontend/FrontendActions.cpp#L614 ) which can play similar role as Clang TargetInfo IMO.

That's why I decided to implement getExplicitAndImplicitAMDGPUTargetFeatures function which performs initialization of default AMDGPU target features.

awarzynski added inline comments.Mar 26 2023, 10:15 AM
flang/lib/Frontend/FrontendActions.cpp
139–142

Thanks for this comprehensive overview! It would be helpful if this rationale was included in the summary (in the spirit of documenting things for our future selves).

So, there isn't anything special about AMDGPU here, is there? We will have to implement similar hooks for other targets at some point too, right? Or perhaps there's some reason to do this for AMDGPU sooner rather than later?

I'm not against this change, just want to better understand the wider context.

domada updated this revision to Diff 508618.Mar 27 2023, 6:13 AM
domada retitled this revision from [Flang][AMDGPU] Add support for AMDGPU to Flang driver to [Clang][Flang][AMDGPU] Add support for AMDGPU to Flang driver.
domada edited the summary of this revision. (Show Details)

Rebase & applied review remarks

domada added inline comments.Mar 27 2023, 6:27 AM
flang/lib/Frontend/FrontendActions.cpp
139–142

Hi,
I modified the comment above to be more informative.

IMO, we need to add similar hooks for other targets. For example:

clang --target=aarch64 t.c -S -emit-llvm -v 
// I see in the logs explicit target features:
// +neon,  +v8a ,  -fmv
// However, generated t.ll contains 4 target features:
"target-features"="+fp-armv8,+neon,+v8a,-fmv"
// It looks like target feature +fp-armv8 is implicit

LGTM about the AMDGPU TargetInfo change.

Do you want to move the AMDGPU changes into AMDGPU.cpp next to AMD.cpp? From the conversation, there seems to be more target specific behaviours.

Do you want to move the AMDGPU changes into AMDGPU.cpp next to AMD.cpp? From the conversation, there seems to be more target specific behaviours.

I prefer to defer further refactoring to the future, so that downstream branches have time to digest this change first.

domada updated this revision to Diff 508660.Mar 27 2023, 8:01 AM

Patch rebased

Thanks for the updates, mostly looks good. Just a couple of extra questions about the test coverage.

flang/lib/Frontend/FrontendActions.cpp
106–110
108

Are you able to add a test that will trigger this?

139–142

Thanks for checking!

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

Hm, there aren't any "implicit" target features here.

Do you want to move the AMDGPU changes into AMDGPU.cpp next to AMD.cpp? From the conversation, there seems to be more target specific behaviours.

@tschuett No. I don't plan to further refactor AMDGPU::TargeTInfo Clang class. My current goal is to add function attributes to the LLVM IR for Fortran OpenMP code and I don't need to make more changes in Clang to finish my goal.

I wanted to ask whether you want to put an AMDGPU.cpp and AMD.cpp file in the flang/lib/Frontend directory.

domada updated this revision to Diff 508977.Mar 28 2023, 5:57 AM

Patch rebased and added new test for checking incorrect wavefront sizes AMDGPU target features.

domada marked 2 inline comments as done.Mar 28 2023, 6:00 AM
domada added inline comments.
flang/lib/Frontend/FrontendActions.cpp
108

Yes, please check the newest patch.

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

You will see them in MLIR code:
https://reviews.llvm.org/D146612#change-ciOHRHlq0yvL (file: mlir/test/Target/LLVMIR/openmp-llvm.mlir )
Clang also does not list these features in command line for AMDGPU. Slightly different situation is for example for ARM target. Clang list 3 options as -cc1 options and it attaches 4 target options to the generated LLVM IR:

clang --target=aarch64 t.c -S -emit-llvm -v 
// I see in the command line 3 explicit target features:
// +neon,  +v8a ,  -fmv
// However, generated t.ll contains 4 target features:
"target-features"="+fp-armv8,+neon,+v8a,-fmv"
// It looks like target feature +fp-armv8 is implicit
domada marked an inline comment as done.Mar 28 2023, 6:05 AM

I wanted to ask whether you want to put an AMDGPU.cpp and AMD.cpp file in the flang/lib/Frontend directory.

@tschuett No, I don't plan to modify further flang/lib/Frontend.

awarzynski accepted this revision.Mar 28 2023, 6:21 AM

Thanks for implementing this, LGTM!

This revision is now accepted and ready to land.Mar 28 2023, 6:21 AM