[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

421

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

3203

Ditto

clang/lib/Frontend/CompilerInvocation.cpp

1994

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

421

will do

clang/include/clang/Driver/Options.td

3203

will do

clang/lib/Frontend/CompilerInvocation.cpp

1994

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

1951

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

45–48

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

428–429

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

[yaxunl]

clang/lib/Frontend/CompilerInvocation.cpp

1951

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

1951

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

1951

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

130–131 ↗

(On Diff #338975)

This seems like a confusing message. Equivalent of what?

clang/include/clang/Driver/Options.td

3176

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

130–131 ↗

(On Diff #338975)

will fix

clang/include/clang/Driver/Options.td

3176

will do

clang/test/CodeGenOpenCL/amdgpu-ieee.cl

21

will fix

[yaxunl]

ping

[arsenm]

clang/include/clang/Basic/DiagnosticDriverKinds.td

131–133 ↗

(On Diff #339092)

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

131–133 ↗

(On Diff #339092)

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

3179

Maybe also note this is an ABI changing option

[yaxunl]

clang/include/clang/Driver/Options.td

3179

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)
 
+/// 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/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)">;
 
+def mamdgpu_ieee : Flag<["-"], "mamdgpu-ieee">, Flags<[CC1Option]>,
+  Group<m_Group>, HelpText<"Floating point opcodes that support exception flag "
+   "gathering quiet and propagate signaling NaN inputs per IEEE 754-2008 (AMDGPU only)">;
+def mno_amdgpu_ieee : Flag<["-"], "mno-amdgpu-ieee">, Flags<[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;
 
+  // When NaN is not honored, floating point opcodes that support exception
+  // flag gathering does not need to quiet or propagate signaling NaN inputs
+  // per IEEE 754-2008. Note this only concerns about signaling NaN.
+  Opts.EmitIEEENaNCompliantInsts =
+      Args.hasFlag(options::OPT_mamdgpu_ieee, options::OPT_mno_amdgpu_ieee,
+                   !LangOptsRef.NoHonorNaNs);
+
   return Diags.getNumErrors() == NumErrorsBefore;
 }
 
 static void
 GenerateDependencyOutputArgs(const DependencyOutputOptions &Opts,
                              SmallVectorImpl<const char *> &Args,
                              CompilerInvocation::StringAllocator SA) {
   const DependencyOutputOptions &DependencyOutputOpts = Opts;
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:   -mamdgpu-ieee | FileCheck -check-prefixes=COMMON,ON %s
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O0 -emit-llvm -o - %s \
+// RUN:   -mno-amdgpu-ieee | FileCheck -check-prefixes=COMMON,OFF %s
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O0 -emit-llvm -o - %s \
+// RUN:   -menable-no-nans | FileCheck -check-prefixes=COMMON,OFF %s
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O0 -emit-llvm -o - %s \
+// RUN:   -menable-no-nans -mamdgpu-ieee | FileCheck -check-prefixes=COMMON,ON %s
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O0 -emit-llvm -o - %s \
+// RUN:   -cl-fast-relaxed-math | FileCheck -check-prefixes=COMMON,OFF %s
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O0 -emit-llvm -o - %s \
+// RUN:   -cl-fast-relaxed-math -mamdgpu-ieee \
+
+// Check AMDGCN ISA generation.
+
+// RUN: | FileCheck -check-prefixes=COMMON,ON %s
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O3 -S -o - %s \
+// RUN:   -mamdgpu-ieee \
+// RUN: | FileCheck -check-prefixes=ISA-ON %s
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -O3 -S -o - %s \
+// RUN:   -mno-amdgpu-ieee \
+// RUN: | FileCheck -check-prefixes=ISA-OFF %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"
+// ON-NOT: attributes [[ATTRS2]] = {{.*}} "amdgpu-ieee"
+// OFF: attributes [[ATTRS2]] = {{.*}} "amdgpu-ieee"="false"