[HIP] Add option -fgpu-inline-threshold

[yaxunl]

Summary

Add option -fgpu-inline-threshold for inline threshold for device compilation only.

[tra]

I'm concerned that if we make it a top-level option, it's likely to be cargo-culted and (mis)used as a sledgehammer in cases where it's not needed. I think the option should remain hidden.

While thresholds do need to be tweaked, in my experience it happens very rarely.
When it does, most of the time it's sufficient to apply __attribute__((always_inline)) to a few functions where it matters.
If AMDGPU bumps into the limit too often, perhaps it's the default threshold value that needs to be changed.

If we do add an option to control inlining threshold, then we should also consider doing the same for other thresholds.
For instance, loop unrolling thresholds in my experience need bumping up about as often as the inlining ones.
Similarly, most of the time the issue can be dealt with at the source code level with #pragma unroll.

Perhaps we should generalize this patch to deal with wider range of threshold.
E.g. we could have something like --gpu-threshold threshold-kind=x which would expand to appropriate cc1 options for GPU sub-compilations.
It would also be nice to handle it in a way that can be used by both CUDA and HIP w/o having to copy/paste code.

Also, this patch would not be necessary if we had the generalized way to specify options for particular offload targets. Alas, we don't have it yet.

[yaxunl]

In D99233#2656446, @tra wrote:

I'm concerned that if we make it a top-level option, it's likely to be cargo-culted and (mis)used as a sledgehammer in cases where it's not needed. I think the option should remain hidden.

While thresholds do need to be tweaked, in my experience it happens very rarely.
When it does, most of the time it's sufficient to apply __attribute__((always_inline)) to a few functions where it matters.
If AMDGPU bumps into the limit too often, perhaps it's the default threshold value that needs to be changed.

Currently ROCm builds all math libs and frameworks with an LLVM option which inline all functions for AMDGPU target. We cannot simply remove that option and use the default inline threshold since it will cause performance degradations. We cannot use -mllvm -inline-threshold=x directly either since it will affect both host and device compilation. We need an option to set the inline threshold for GPU only so that we could fine-tuning the inline threshold. I agree that this option should be hidden since it is intended for compiler development.

If we do add an option to control inlining threshold, then we should also consider doing the same for other thresholds.
For instance, loop unrolling thresholds in my experience need bumping up about as often as the inlining ones.
Similarly, most of the time the issue can be dealt with at the source code level with #pragma unroll.

Perhaps we should generalize this patch to deal with wider range of threshold.
E.g. we could have something like --gpu-threshold threshold-kind=x which would expand to appropriate cc1 options for GPU sub-compilations.

My concern with --gpu-threshold threshold-kind=x is that it needs custom handling, e.g. setting default values, letting the last option win if multiple options are set. Using separate options allows standard handling of these options.

It would also be nice to handle it in a way that can be used by both CUDA and HIP w/o having to copy/paste code.

I can add a function as ToolChain so that it can be used by different ToolChains.

Also, this patch would not be necessary if we had the generalized way to specify options for particular offload targets. Alas, we don't have it yet.

The planned new option for offloading will be a more generic solution, however, I expect it will take time to develop and be adopted.

[tra]

The planned new option for offloading will be a more generic solution, however, I expect it will take time to develop and be adopted.

Agreed. OK, let's use a hidden option until we have a better way of dealing with this.

clang/include/clang/Driver/Options.td

960

This option is only handled at the top level, it does not need CC1Option. It does need HelpHidden.

Also, the option should probably be -fgpu-inline-threshold=... as it's a parameter tweak, and not something more serious, like --offload-arch.
Naming is hard. :-)

[yaxunl]

clang/include/clang/Driver/Options.td

960

will do

[yaxunl]

revised by Artem's comments.

clang/include/clang/Driver/Options.td

defm gpu_exclude_wrong_side_overloads : BoolFOption<"gpu-exclude-wrong-side-overloads",
   PosFlag<SetTrue, [CC1Option], "Always exclude wrong side overloads">,
   NegFlag<SetFalse, [], "Exclude wrong side overloads only if there are same side overloads">,
   BothFlags<[HelpHidden], " in overloading resolution for CUDA/HIP">>;
 def gpu_max_threads_per_block_EQ : Joined<["--"], "gpu-max-threads-per-block=">,
   Flags<[CC1Option]>,
   HelpText<"Default max threads per block for kernel launch bounds for HIP">,
   MarshallingInfoInt<LangOpts<"GPUMaxThreadsPerBlock">, "1024">,
   ShouldParseIf<hip.KeyPath>;
+def fgpu_inline_threshold_EQ : Joined<["-"], "fgpu-inline-threshold=">,
+  Flags<[HelpHidden]>,
+  HelpText<"Inline threshold for device compilation for CUDA/HIP">;
 def gpu_instrument_lib_EQ : Joined<["--"], "gpu-instrument-lib=">,
   HelpText<"Instrument device library for HIP, which is a LLVM bitcode containing "
   "__cyg_profile_func_enter and __cyg_profile_func_exit">;
 defm gpu_sanitize : BoolFOption<"gpu-sanitize", EmptyKPM, DefaultFalse,
   PosFlag<SetTrue, [], "Enable sanitizer for AMDGPU target">,
   NegFlag<SetFalse>>;
 def cuid_EQ : Joined<["-"], "cuid=">, Flags<[CC1Option]>,
   HelpText<"An ID for compilation unit, which should be the same for the same "

clang/lib/Driver/ToolChains/Clang.cpp

if (IsCuda || IsHIP) {
     auto CUID = cast<InputAction>(SourceAction)->getId();
     if (!CUID.empty())
       CmdArgs.push_back(Args.MakeArgString(Twine("-cuid=") + Twine(CUID)));
   }
 
   if (IsHIP)
     CmdArgs.push_back("-fcuda-allow-variadic-functions");
 
+  if (IsCudaDevice || IsHIPDevice) {
+    StringRef InlineThresh =
+        Args.getLastArgValue(options::OPT_fgpu_inline_threshold_EQ);
+    if (!InlineThresh.empty()) {
+      std::string ArgStr =
+          std::string("-inline-threshold=") + InlineThresh.str();
+      CmdArgs.append({"-mllvm", Args.MakeArgStringRef(ArgStr)});
+    }
+  }
+
   // OpenMP offloading device jobs take the argument -fopenmp-host-ir-file-path
   // to specify the result of the compile phase on the host, so the meaningful
   // device declarations can be identified. Also, -fopenmp-is-device is passed
   // along to tell the frontend that it is generating code for a device, so that
   // only the relevant declarations are emitted.
   if (IsOpenMPDevice) {
     CmdArgs.push_back("-fopenmp-is-device");
     if (OpenMPDeviceInput) {

clang/test/Driver/hip-options.hip

 // FIX-OVERLOAD: clang{{.*}} "-triple" "x86_64-unknown-linux-gnu" {{.*}} "-fgpu-exclude-wrong-side-overloads" "-fgpu-defer-diag"
 
 // Check -mconstructor-aliases is not passed to device compilation.
 
 // RUN: %clang -### -target x86_64-unknown-linux-gnu -nogpuinc -nogpulib \
 // RUN:   --cuda-gpu-arch=gfx906  %s 2>&1 | FileCheck -check-prefix=CTA %s
 // CTA: clang{{.*}} "-triple" "x86_64-unknown-linux-gnu" {{.*}} "-mconstructor-aliases"
 // CTA-NOT: clang{{.*}} "-triple" "amdgcn-amd-amdhsa" {{.*}} "-mconstructor-aliases"
+
+// RUN: %clang -### -target x86_64-unknown-linux-gnu -nogpuinc -nogpulib \
+// RUN:   --offload-arch=gfx906 -fgpu-inline-threshold=1000 %s 2>&1 | FileCheck -check-prefix=THRESH %s
+// THRESH: clang{{.*}} "-triple" "amdgcn-amd-amdhsa" {{.*}} "-mllvm" "-inline-threshold=1000"
+// THRESH-NOT: clang{{.*}} "-triple" "x86_64-unknown-linux-gnu" {{.*}} "-inline-threshold=1000"