[AMDGPU] Add options -mamdgpu-ieee -mno-amdgpu-ieee

[yaxunl]

Summary

AMDGPU backend need to know whether floating point opcodes that support exception
flag gathering quiet and propagate signaling NaN inputs per IEEE754-2008, which is
conveyed by a function attribute "amdgpu-ieee". "amdgpu-ieee"="false" turns this off.
Without this function attribute backend assumes it is on for compute functions.

-mamdgpu-ieee and -mno-amdgpu-ieee are added to Clang to control this function attribute.
By default it is on. -mno-amdgpu-ieee requires -fno-honor-nans or equivalent.

[arsenm]

clang/include/clang/Basic/CodeGenOptions.def

422

Description is misleading. Better description would be the first line from the manual,
"Floating point opcodes that support exception flag gathering quiet and propagate sig- naling NaN inputs per IEEE 754-2008"

clang/include/clang/Driver/Options.td

3211

Ditto

clang/lib/Frontend/CompilerInvocation.cpp

1993

Add a comment explaining why to turn it off? Also should note this is only really concerns signaling nans

clang/test/CodeGenOpenCL/amdgpu-ieee.cl

21

Should also test a non-kernel function

[arsenm]

clang/test/CodeGenOpenCL/amdgpu-ieee.cl

21

I think we should also have some ISA check run lines that show the final result for minnum/maxnum lowering, as well as if output modifiers are used

[yaxunl]

This comment was removed by yaxunl.

[yaxunl]

clang/include/clang/Basic/CodeGenOptions.def

422

will do

clang/include/clang/Driver/Options.td

3211

will do

clang/lib/Frontend/CompilerInvocation.cpp

1993

will do

clang/test/CodeGenOpenCL/amdgpu-ieee.cl

21

will do

[yaxunl]

Revised by Matt's comments.

[yaxunl]

rebase

[arsenm]

clang/lib/Frontend/CompilerInvocation.cpp

1952

This should not be implied by no honor nans. This is an ABI changing option. We need to require that no nans FP math is enabled to use this mode correctly though

clang/test/CodeGenOpenCL/amdgpu-ieee.cl

46–49

Should also check the fast math attributes

[yaxunl]

The recent change https://reviews.llvm.org/D96280 caused some difficulty for this patch. I would like to have some suggestions.

Basically current FE requires any codegen or target option should be uniquely reproduced from its internal representation. The current patch does not satisfy the requirement. That's why it fails the pre-merge check.

Currently, the internal representation CodeGenOpt.EmitIEEENaNCompliantInsts is not uniquely determined by the command line options -m[no]-amdgpu-ieee. It is also determined by LangOptsRef.NoHonorNaNs. Therefore I cannot regenerate the original command line option by the value of CodeGenOpt.EmitIEEENaNCompliantInsts.

One solution I can think of, is to use -mamdgpu-ieee={default|on|off}. This way I should be able to reproduce the original command line.

Any comments? Thanks.

[arsenm]

In D77013#2702129, @yaxunl wrote:

The recent change https://reviews.llvm.org/D96280 caused some difficulty for this patch. I would like to have some suggestions.

Basically current FE requires any codegen or target option should be uniquely reproduced from its internal representation. The current patch does not satisfy the requirement. That's why it fails the pre-merge check.

Currently, the internal representation CodeGenOpt.EmitIEEENaNCompliantInsts is not uniquely determined by the command line options -m[no]-amdgpu-ieee. It is also determined by LangOptsRef.NoHonorNaNs. Therefore I cannot regenerate the original command line option by the value of CodeGenOpt.EmitIEEENaNCompliantInsts.

These are two independent options. -mno-amdgpu-ieee should require no-nans-fp-math to be used by the driver. The -m flag is an ABI changing option, and we don't implement the semantics required to make it work without knowing there are no signaling nans

One solution I can think of, is to use -mamdgpu-ieee={default|on|off}. This way I should be able to reproduce the original command line.

The default should only be on. Why is this not a problem for every other -m/-mno flag?

clang/include/clang/Basic/CodeGenOptions.def

429–430

This should explain this is setting a bit in the expected default floating point mode register

[yaxunl]

clang/lib/Frontend/CompilerInvocation.cpp

1952

OK I plan to make changes about this. However I need to clarify about when should we add function attribute amdgpu-ieee=false:

1. when neither -mamdgpu-ieee nor -mno-amdgpu-ieee is specified, we will add func attr amdgpu-ieee=false if LangOptsRef.NoHonorNaNs is true

2. when -mamdgpu-ieee or -mno-amdgpu-ieee is specified, we will add func attr amdgpu-ieee=false solely based on these two options, disregarding LangOptsRef.NoHonorNaNs

Is that correct?

[arsenm]

clang/lib/Frontend/CompilerInvocation.cpp

1952

1. You should only add the attribute "amdgpu-ieee"="false" if and only if -mno-amdgpu-ieee is used. This is entirely orthogonal to NoHonorNaNs. The default is amdgpu-ieee=true, and there's no need to explicitly mark the function
2. It is an error to use -mno-amdgpu-ieee without NoHonorNaNs because we do not implement the legalization required to manually quiet signaling nans. We cannot silently drop the use of of this

[yaxunl]

clang/lib/Frontend/CompilerInvocation.cpp

1952

I can add diagnostics in clang -cc1 when -mno-amdgpu-ieee is used without NoHonorNaNs. I cannot diagnose it in clang driver because there are multiple driver options affecting NoHonorNaNs and they could override each other.

[yaxunl]

revised by Matt's comments

[arsenm]

clang/include/clang/Basic/DiagnosticDriverKinds.td

132–133

This seems like a confusing message. Equivalent of what?

clang/include/clang/Driver/Options.td

3182

s/Setting a bit/Sets the IEEE bit/

clang/test/CodeGenOpenCL/amdgpu-ieee.cl

21

Typo Hornor

[yaxunl]

revised by Matt's comments

clang/include/clang/Basic/DiagnosticDriverKinds.td

132–133

will fix

clang/include/clang/Driver/Options.td

3182

will do

clang/test/CodeGenOpenCL/amdgpu-ieee.cl

21

will fix

[yaxunl]

ping

[arsenm]

clang/include/clang/Basic/DiagnosticDriverKinds.td

133–135

e.g. and a list of flags seems weird for an error message. How about "only allowed with relaxed NaN handling"?

[yaxunl]

clang/include/clang/Basic/DiagnosticDriverKinds.td

133–135

sounds good. will do. I think users should be able to look up clang options which relax NaN handling.

[yaxunl]

revised by Matt's comments

[arsenm]

clang/include/clang/Driver/Options.td

3185

Maybe also note this is an ABI changing option

[yaxunl]

clang/include/clang/Driver/Options.td

3185

will do

[yaxunl]

updated help text and test

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(Addrsig, 1, 0)
 
 /// Whether to emit unused static constants.
 CODEGENOPT(KeepStaticConsts, 1, 0)
 
 /// Whether to follow the AAPCS enforcing at least one read before storing to a volatile bitfield
 CODEGENOPT(ForceAAPCSBitfieldLoad, 1, 0)
 
 /// Assume that by-value parameters do not alias any other values.
 CODEGENOPT(PassByValueIsNoAlias, 1, 0)
 
 /// Whether to not follow the AAPCS that enforces volatile bit-field access width to be
 /// according to the field declaring type width.
 CODEGENOPT(AAPCSBitfieldWidth, 1, 1)
 
+/// Sets the IEEE bit in the expected default floating point mode register.
+/// Floating point opcodes that support exception flag gathering quiet and
+/// propagate signaling NaN inputs per IEEE 754-2008 (AMDGPU Only)
+CODEGENOPT(EmitIEEENaNCompliantInsts, 1, 1)
+
 #undef CODEGENOPT
 #undef ENUM_CODEGENOPT
 #undef VALUE_CODEGENOPT

clang/include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_invalid_Xarch_argument_with_args : Error<
   "invalid Xarch argument: '%0', options requiring arguments are unsupported">;
 def err_drv_Xopenmp_target_missing_triple : Error<
   "cannot deduce implicit triple value for -Xopenmp-target, specify triple using -Xopenmp-target=<triple>">;
 def err_drv_invalid_Xopenmp_target_with_args : Error<
   "invalid -Xopenmp-target argument: '%0', options requiring arguments are unsupported">;
 def err_drv_argument_only_allowed_with : Error<
   "invalid argument '%0' only allowed with '%1'">;
+def err_drv_amdgpu_ieee_without_no_honor_nans : Error<
+  "invalid argument '-mno-amdgpu-ieee' only allowed with relaxed NaN handling">;
 def err_drv_argument_not_allowed_with : Error<
   "invalid argument '%0' not allowed with '%1'">;
 def err_drv_invalid_version_number : Error<
   "invalid version number in '%0'">;
 def err_drv_no_linker_llvm_support : Error<
   "'%0': unable to pass LLVM bit-code files to linker">;
 def err_drv_no_ast_support : Error<
   "'%0': unable to use AST files with this tool">;
 def err_drv_no_module_support : Error<
   "'%0': unable to use module files with this tool">;

clang/include/clang/Driver/Options.td

 def mtail_call : Flag<["-"], "mtail-call">, Group<m_wasm_Features_Group>;
 def mno_tail_call : Flag<["-"], "mno-tail-call">, Group<m_wasm_Features_Group>;
 def mreference_types : Flag<["-"], "mreference-types">, Group<m_wasm_Features_Group>;
 def mno_reference_types : Flag<["-"], "mno-reference-types">, Group<m_wasm_Features_Group>;
 def mexec_model_EQ : Joined<["-"], "mexec-model=">, Group<m_wasm_Features_Driver_Group>,
                      Values<"command,reactor">,
                      HelpText<"Execution model (WebAssembly only)">;
 
+defm amdgpu_ieee : BoolOption<"m", "amdgpu-ieee",
+  CodeGenOpts<"EmitIEEENaNCompliantInsts">, DefaultTrue,
+  PosFlag<SetTrue, [], "Sets the IEEE bit in the expected default floating point "
+  " mode register. Floating point opcodes that support exception flag "
+  "gathering quiet and propagate signaling NaN inputs per IEEE 754-2008. "
+  "This option changes the ABI. (AMDGPU only)">,
+  NegFlag<SetFalse, [CC1Option]>>, Group<m_Group>;
+
 def mcode_object_version_EQ : Joined<["-"], "mcode-object-version=">, Group<m_Group>,
   HelpText<"Specify code object ABI version. Defaults to 3. (AMDGPU only)">,
   MetaVarName<"<version>">, Values<"2,3,4">;
 
 defm code_object_v3_legacy : SimpleMFlag<"code-object-v3",
   "Legacy option to specify code object ABI V3",
   "Legacy option to specify code object ABI V2",
   " (AMDGPU only)">;
 defm cumode : SimpleMFlag<"cumode",
   "Specify CU wavefront", "Specify WGP wavefront",
   " execution mode (AMDGPU only)", m_amdgpu_Features_Group>;
 defm tgsplit : SimpleMFlag<"tgsplit", "Enable", "Disable",
   " threadgroup split execution mode (AMDGPU only)", m_amdgpu_Features_Group>;
 defm wavefrontsize64 : SimpleMFlag<"wavefrontsize64",
   "Specify wavefront size 64", "Specify wavefront size 32",
   " mode (AMDGPU only)">;
 
 defm unsafe_fp_atomics : BoolOption<"m", "unsafe-fp-atomics",
   TargetOpts<"AllowAMDGPUUnsafeFPAtomics">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable unsafe floating point atomic instructions (AMDGPU only)">,
   NegFlag<SetFalse>>, Group<m_Group>;
 
 def faltivec : Flag<["-"], "faltivec">, Group<f_Group>, Flags<[NoXarchOption]>;
 def fno_altivec : Flag<["-"], "fno-altivec">, Group<f_Group>, Flags<[NoXarchOption]>;
 def maltivec : Flag<["-"], "maltivec">, Group<m_ppc_Features_Group>;
 def mno_altivec : Flag<["-"], "mno-altivec">, Group<m_ppc_Features_Group>;
 def mpcrel: Flag<["-"], "mpcrel">, Group<m_ppc_Features_Group>;
 def mno_pcrel: Flag<["-"], "mno-pcrel">, Group<m_ppc_Features_Group>;
 def mspe : Flag<["-"], "mspe">, Group<m_ppc_Features_Group>;
 def mno_spe : Flag<["-"], "mno-spe">, Group<m_ppc_Features_Group>;
 def mefpu2 : Flag<["-"], "mefpu2">, Group<m_ppc_Features_Group>;
 def mabi_EQ_vec_extabi : Flag<["-"], "mabi=vec-extabi">, Group<m_Group>, Flags<[CC1Option]>,

clang/lib/CodeGen/TargetInfo.cpp

if (const auto *Attr = FD->getAttr<AMDGPUNumVGPRAttr>()) {
     uint32_t NumVGPR = Attr->getNumVGPR();
 
     if (NumVGPR != 0)
       F->addFnAttr("amdgpu-num-vgpr", llvm::utostr(NumVGPR));
   }
 
   if (M.getContext().getTargetInfo().allowAMDGPUUnsafeFPAtomics())
     F->addFnAttr("amdgpu-unsafe-fp-atomics", "true");
+
+  if (!getABIInfo().getCodeGenOpts().EmitIEEENaNCompliantInsts)
+    F->addFnAttr("amdgpu-ieee", "false");
 }
 
 unsigned AMDGPUTargetCodeGenInfo::getOpenCLKernelCallingConv() const {
   return llvm::CallingConv::AMDGPU_KERNEL;
 }
 
 // Currently LLVM assumes null pointers always have value 0,
 // which results in incorrectly transformed IR. Therefore, instead of

clang/lib/Frontend/CompilerInvocation.cpp

#undef CODEGEN_OPTION_WITH_MARSHALLING
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
   if (Args.hasArg(options::OPT_ffinite_loops))
     Opts.FiniteLoops = CodeGenOptions::FiniteLoopsKind::Always;
   else if (Args.hasArg(options::OPT_fno_finite_loops))
     Opts.FiniteLoops = CodeGenOptions::FiniteLoopsKind::Never;
 
+  Opts.EmitIEEENaNCompliantInsts =
+      Args.hasFlag(options::OPT_mamdgpu_ieee, options::OPT_mno_amdgpu_ieee);
+  if (!Opts.EmitIEEENaNCompliantInsts && !LangOptsRef.NoHonorNaNs)
+    Diags.Report(diag::err_drv_amdgpu_ieee_without_no_honor_nans);
+
   return Diags.getNumErrors() == NumErrorsBefore;
 }
 
 static void
 GenerateDependencyOutputArgs(const DependencyOutputOptions &Opts,
                              SmallVectorImpl<const char *> &Args,
                              CompilerInvocation::StringAllocator SA) {
   const DependencyOutputOptions &DependencyOutputOpts = Opts;
 #define DEPENDENCY_OUTPUT_OPTION_WITH_MARSHALLING(                             \
case EDK_ProfileList:
       // marshalling infrastructure.
       continue;
     case EDK_DepFileEntry:
       GenerateArg(Args, OPT_fdepfile_entry, Dep.first, SA);
       break;
     }
   }
 }
 
 static bool ParseDependencyOutputArgs(DependencyOutputOptions &Opts,
                                       ArgList &Args, DiagnosticsEngine &Diags,
                                       frontend::ActionKind Action,
                                       bool ShowLineMarkers) {
   unsigned NumErrorsBefore = Diags.getNumErrors();
 
   DependencyOutputOptions &DependencyOutputOpts = Opts;
 #define DEPENDENCY_OUTPUT_OPTION_WITH_MARSHALLING(                             \

clang/test/CodeGenOpenCL/amdgpu-ieee.cl

+// REQUIRES: amdgpu-registered-target
+//
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O0 -emit-llvm -o - %s \
+// RUN:   | FileCheck -check-prefixes=COMMON,ON %s
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O0 -emit-llvm -o - %s \
+// RUN:   -mno-amdgpu-ieee -menable-no-nans \
+// RUN:   | FileCheck -check-prefixes=COMMON,OFF %s
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O0 -emit-llvm -o - %s \
+// RUN:   -mno-amdgpu-ieee -cl-fast-relaxed-math \
+// RUN:   | FileCheck -check-prefixes=COMMON,OFF %s
+
+// Check AMDGCN ISA generation.
+
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O3 -S -o - %s \
+// RUN:   | FileCheck -check-prefixes=ISA-ON %s
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O3 -S -o - %s \
+// RUN:   -mno-amdgpu-ieee -menable-no-nans \
+// RUN:   | FileCheck -check-prefixes=ISA-OFF %s
+
+// Check diagnostics when using -mno-amdgpu-ieee without NoHonorNaNs.
+
+// RUN: not %clang_cc1 -triple amdgcn-amd-amdhsa -O0 -emit-llvm -o - %s \
+// RUN:   -mno-amdgpu-ieee 2>&1 | FileCheck -check-prefixes=DIAG %s
+
+// COMMON: define{{.*}} amdgpu_kernel void @kern{{.*}} [[ATTRS1:#[0-9]+]]
+// ISA-ON: v_mul_f32_e64 v{{[0-9]+}}, 1.0, s{{[0-9]+}}
+// ISA-ON: v_mul_f32_e64 v{{[0-9]+}}, 1.0, s{{[0-9]+}}
+// ISA-ON: v_min_f32_e32
+// ISA-ON: ; IeeeMode: 1
+// ISA-OFF-NOT: v_mul_f32_e64 v{{[0-9]+}}, 1.0, s{{[0-9]+}}
+// ISA-OFF-NOT: v_mul_f32_e64 v{{[0-9]+}}, 1.0, s{{[0-9]+}}
+// ISA-OFF: v_min_f32_e32
+// ISA-OFF: ; IeeeMode: 0
+kernel void kern(global float *x, float y, float z) {
+  *x = __builtin_fmin(y, z);
+}
+
+// COMMON: define{{.*}}void @fun() [[ATTRS2:#[0-9]+]]
+void fun() {
+}
+
+// ON-NOT: attributes [[ATTRS1]] = {{.*}} "amdgpu-ieee"
+// OFF: attributes [[ATTRS1]] = {{.*}} "amdgpu-ieee"="false"{{.*}}"no-nans-fp-math"="true"{{.*}}"no-trapping-math"="true"
+// ON-NOT: attributes [[ATTRS2]] = {{.*}} "amdgpu-ieee"
+// OFF: attributes [[ATTRS2]] = {{.*}} "amdgpu-ieee"="false"{{.*}}"no-nans-fp-math"="true"{{.*}}"no-trapping-math"="true"
+
+// DIAG: invalid argument '-mno-amdgpu-ieee' only allowed with relaxed NaN handling