Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLANG] Full support of complex multiplication and division. #81514

Merged
merged 23 commits into from
Mar 20, 2024
Merged

[CLANG] Full support of complex multiplication and division. #81514

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
13fd739
[CLANG] Full support of complex multiplication and division.
zahiraam Feb 12, 2024
eb9a35c
Changed the names of the values for the option and added
zahiraam Feb 20, 2024
d3c4f78
Merge remote-tracking branch 'origin/main' into ComplexRange
zahiraam Feb 20, 2024
4aa0925
Fix LIT tests.
zahiraam Feb 21, 2024
fcd5665
Merge remote-tracking branch 'origin/main' into ComplexRange
zahiraam Feb 21, 2024
2ddba9a
Addressed only a few issue from reviewer's comment.
zahiraam Feb 22, 2024
e62c462
Fixed warnings.
zahiraam Feb 26, 2024
f635f94
Merge branch 'llvm:main' into ComplexRange
zahiraam Feb 26, 2024
1d61aa6
Fix format.
zahiraam Feb 26, 2024
148b6ce
Merge branch 'ComplexRange' of https://github.com/zahiraam/llvm-proje…
zahiraam Feb 26, 2024
5aa2711
Fix format.
zahiraam Feb 26, 2024
ba9a8da
Fixed LIT test nofpclass.c and fixed the type promotion.
zahiraam Feb 27, 2024
9098908
Addressed review comments. Fixed the warnings code in Clang.cpp
zahiraam Feb 27, 2024
52181c7
Added more tests to cx-complex-range.c
zahiraam Feb 28, 2024
a9449de
Set the default value of option to full.
zahiraam Feb 29, 2024
e20741e
Wrote a more general function to deal with next larger types
zahiraam Mar 4, 2024
0d97b9b
Addressed review comments.
zahiraam Mar 11, 2024
bc3fa4f
Fix format.
zahiraam Mar 11, 2024
ec6296f
Merge branch 'main' into ComplexRange
zahiraam Mar 12, 2024
3117dbd
Addressed some of the review comments and working on the rest.
zahiraam Mar 15, 2024
0a08598
Addressed a few more comments. The last issue still WIP is the
zahiraam Mar 15, 2024
a558d31
Added LIT tests.
zahiraam Mar 18, 2024
dec045b
Changed the type of FPHasBeenPromoted to bool.
zahiraam Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 38 additions & 7 deletions clang/docs/UsersManual.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1847,19 +1847,50 @@ floating point semantic models: precise (the default), strict, and fast.
* ``16`` - Forces ``_Float16`` operations to be emitted without using excess
precision arithmetic.

.. option:: -fcomplex-arithmetic=<value>:

This option specifies the implementation for complex multiplication and division.

Valid values are: ``basic``, ``improved``, ``full`` and ``promoted``.

* ``basic`` Implementation of complex division and multiplication using
algebraic formulas at source precision. No special handling to avoid
overflow. NaN and infinite values are not handled.
* ``improved`` Implementation of complex division using the Smith algorithm
at source precision. Smith's algorithm for complex division.
See SMITH, R. L. Algorithm 116: Complex division. Commun. ACM 5, 8 (1962).
This value offers improved handling for overflow in intermediate
calculations, but overflow may occur. NaN and infinite values are not
handled in some cases.
* ``full`` Implementation of complex division and multiplication using a
call to runtime library functions (generally the case, but the BE might
sometimes replace the library call if it knows enough about the potential
range of the inputs). Overflow and non-finite values are handled by the
library implementation. For the case of multiplication overflow will occur in
accordance with normal floating-point rules. This is the default value.
* ``promoted`` Implementation of complex division using algebraic formulas at
higher precision. Overflow is handled. Non-finite values are handled in some
cases. If the target does not have native support for a higher precision
data type, the implementation for the complex operation using the Smith
algorithm will be used. Overflow may still occur in some cases. NaN and
infinite values are not handled.

.. option:: -fcx-limited-range:

This option enables the naive mathematical formulas for complex division and
multiplication with no NaN checking of results. The default is
``-fno-cx-limited-range``, but this option is enabled by the ``-ffast-math``
This option is aliased to ``-fcomplex-arithmetic=basic``. It enables the
naive mathematical formulas for complex division and multiplication with no
NaN checking of results. The default is ``-fno-cx-limited-range`` aliased to
``-fcomplex-arithmetic=full``. This option is enabled by the ``-ffast-math``
option.

.. option:: -fcx-fortran-rules:

This option enables the naive mathematical formulas for complex
multiplication and enables application of Smith's algorithm for complex
division. See SMITH, R. L. Algorithm 116: Complex division. Commun.
ACM 5, 8 (1962). The default is ``-fno-cx-fortran-rules``.
This option is aliased to ``-fcomplex-arithmetic=improved``. It enables the
naive mathematical formulas for complex multiplication and enables application
of Smith's algorithm for complex division. See SMITH, R. L. Algorithm 116:
Complex division. Commun. ACM 5, 8 (1962).
The default is ``-fno-cx-fortran-rules`` aliased to
``-fcomplex-arithmetic=full``.

.. _floating-point-environment:

Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/Basic/DiagnosticCommonKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ def note_using : Note<"using">;
def note_possibility : Note<"one possibility">;
def note_also_found : Note<"also found">;

def warn_next_larger_fp_type_same_size_than_fp : Warning<
"higher precision floating-point type size has the same size than "
"floating-point type size">,
InGroup<DiagGroup<"higher-precision-fp">>;

// Parse && Lex

let CategoryName = "Lexical or Preprocessor Issue" in {
Expand Down
33 changes: 32 additions & 1 deletion clang/include/clang/Basic/LangOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,38 @@ class LangOptionsBase {
IncompleteOnly = 3,
};

enum ComplexRangeKind { CX_Full, CX_Limited, CX_Fortran, CX_None };
/// Controls the various implementations for complex multiplication and
// division.
enum ComplexRangeKind {
/// Implementation of complex division and multiplication using a call to
/// runtime library functions(generally the case, but the BE might
/// sometimes replace the library call if it knows enough about the
/// potential range of the inputs). Overflow and non-finite values are
/// handled by the library implementation. This is the default value.
CX_Full,

/// Implementation of complex division offering an improved handling
/// for overflow in intermediate calculations with no special handling for
/// NaN and infinite values.
CX_Improved,

/// Implementation of complex division using algebraic formulas at
/// higher precision. Overflow is handled. Non-finite values are handled in
/// some cases. If the target hardware does not have native support for a
/// higher precision data type, an implementation for the complex operation
/// will be used to provide improved guards against intermediate overflow,
/// but overflow and underflow may still occur in some cases. NaN and
/// infinite values are not handled.
CX_Promoted,

/// Implementation of complex division and multiplication using
/// algebraic formulas at source precision. No special handling to avoid
/// overflow. NaN and infinite values are not handled.
CX_Basic,

/// No range rule is enabled.
CX_None
};

// Define simple language options (with no accessors).
#define LANGOPT(Name, Bits, Default, Description) unsigned Name : Bits;
Expand Down
25 changes: 15 additions & 10 deletions clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,30 +1046,35 @@ defm offload_uniform_block : BoolFOption<"offload-uniform-block",
NegFlag<SetFalse, [], [ClangOption, CC1Option], "Don't assume">,
BothFlags<[], [ClangOption], " that kernels are launched with uniform block sizes (default true for CUDA/HIP and false otherwise)">>;

def fcx_limited_range : Joined<["-"], "fcx-limited-range">,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize these had made it into the 18.0 release when I suggested that we could remove them. We would need at least one release where they are marked as deprecated, but since they are standard gcc options, maybe it makes sense to just keep them and have them alias to the new option as:

-fcx-limited-range --> -fcomplex-arithmetic=basic
-fcx-fortran-rules --> -fcomplex-arithmetic=improved
-fno-cx-limited-range --> -fcomplex-arithmetic=full
-fno-cx-fortran-rules --> -fcomplex-arithmetic=full

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with aliasing is that the user would be allowed to write something like this:
-fcx-limited-range -fcomplex-arithmetic=improved
This will generate a warning like this:
warning: overriding '-fcomplex-arithmetic=basic' option with '-fcomplex-arithmetic=improved' [-Woverriding-option]

This warning is a bit mis-leading and doesn't reflect the option used in the command line. Not sure this can be corrected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. I meant "aliasing" in the non-technical sense of "having the same meaning." How that gets implemented is another matter. I think the driver could translate them to the same cc1 option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes there is a way of doing that:

def fcx_limited_range : Flag<["-"], "fcx-limited-range">,
  Group<f_Group>, Visibility<[ClangOption, CC1Option]>,
  HelpText<"Basic algebraic expansions of complex arithmetic operations "
           "involving are enabled.">,
  Alias<fcomplex_arithmetic_EQ>, AliasArgs<["basic"]>;

That still produces the misleading warning for: -fcx-limited-range -fcomplex-arithmetic=improved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant to suggest is that you can leave the driver-level options as if they were independent, but when we process them in RenderFloatingPointOptions, -fcx-limited-range and -fcomplex-arithmetic=basic (for example), would add the same cc1 option. Since the warning is generated from the RenderFloatingPointOptions we should be able to make that report the expected output.

def fcomplex_arithmetic_EQ : Joined<["-"], "fcomplex-arithmetic=">, Group<f_Group>,
Visibility<[ClangOption, CC1Option]>,
Values<"full,improved,promoted,basic">, NormalizedValuesScope<"LangOptions">,
NormalizedValues<["CX_Full", "CX_Improved", "CX_Promoted", "CX_Basic"]>;

def complex_range_EQ : Joined<["-"], "complex-range=">, Group<f_Group>,
Visibility<[CC1Option]>,
Values<"full,improved,promoted,basic">, NormalizedValuesScope<"LangOptions">,
NormalizedValues<["CX_Full", "CX_Improved", "CX_Promoted", "CX_Basic"]>,
MarshallingInfoEnum<LangOpts<"ComplexRange">, "CX_Full">;

def fcx_limited_range : Flag<["-"], "fcx-limited-range">,
Group<f_Group>, Visibility<[ClangOption, CC1Option]>,
HelpText<"Basic algebraic expansions of complex arithmetic operations "
"involving are enabled.">;

def fno_cx_limited_range : Joined<["-"], "fno-cx-limited-range">,
def fno_cx_limited_range : Flag<["-"], "fno-cx-limited-range">,
Group<f_Group>, Visibility<[ClangOption, CC1Option]>,
HelpText<"Basic algebraic expansions of complex arithmetic operations "
"involving are disabled.">;

def fcx_fortran_rules : Joined<["-"], "fcx-fortran-rules">,
def fcx_fortran_rules : Flag<["-"], "fcx-fortran-rules">,
Group<f_Group>, Visibility<[ClangOption, CC1Option]>,
HelpText<"Range reduction is enabled for complex arithmetic operations.">;

def fno_cx_fortran_rules : Joined<["-"], "fno-cx-fortran-rules">,
def fno_cx_fortran_rules : Flag<["-"], "fno-cx-fortran-rules">,
Copy link
Contributor

@mdtoguchi mdtoguchi Mar 14, 2024 •
edited by zahiraam
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for fcx_limited_range/fno_cx_limited_range, fcx_fortran_rules/fno_cx_fortran_rules additions, these look like good candidates to use BoolOptionWithoutMarshalling to simplify the representation.
Users should still be able to use fcx-limited-range (and all) on the command line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can BoolOptionWithoutMarshalling be used for clang options?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just set the needed Visibilty values in your PosFlag and NegFlag entries.

Group<f_Group>, Visibility<[ClangOption, CC1Option]>,
HelpText<"Range reduction is disabled for complex arithmetic operations.">;

def complex_range_EQ : Joined<["-"], "complex-range=">, Group<f_Group>,
Visibility<[CC1Option]>,
Values<"full,limited,fortran">, NormalizedValuesScope<"LangOptions">,
NormalizedValues<["CX_Full", "CX_Limited", "CX_Fortran"]>,
MarshallingInfoEnum<LangOpts<"ComplexRange">, "CX_Full">;

// OpenCL-only Options
def cl_opt_disable : Flag<["-"], "cl-opt-disable">, Group<opencl_Group>,
Visibility<[ClangOption, CC1Option]>,
Expand Down
72 changes: 59 additions & 13 deletions clang/lib/CodeGen/CGExprComplex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,12 @@ class ComplexExprEmitter
CGBuilderTy &Builder;
bool IgnoreReal;
bool IgnoreImag;
public:
ComplexExprEmitter(CodeGenFunction &cgf, bool ir=false, bool ii=false)
: CGF(cgf), Builder(CGF.Builder), IgnoreReal(ir), IgnoreImag(ii) {
}
LangOptions::ComplexRangeKind FPHasBeenPromoted;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a somewhat misleading variable name. The name implies that it will be a Boolean value.


public:
ComplexExprEmitter(CodeGenFunction &cgf, bool ir = false, bool ii = false)
: CGF(cgf), Builder(CGF.Builder), IgnoreReal(ir), IgnoreImag(ii),
FPHasBeenPromoted(LangOptions::ComplexRangeKind::CX_None) {}

//===--------------------------------------------------------------------===//
// Utilities
Expand Down Expand Up @@ -287,9 +288,47 @@ class ComplexExprEmitter
ComplexPairTy EmitComplexBinOpLibCall(StringRef LibCallName,
const BinOpInfo &Op);

QualType getPromotionType(QualType Ty) {
QualType GetHigherPrecisionFPType(QualType ElementType) {
const auto *CurrentBT = dyn_cast<BuiltinType>(ElementType);
switch (CurrentBT->getKind()) {
case BuiltinType::Kind::Float16:
return CGF.getContext().FloatTy;
case BuiltinType::Kind::Float:
case BuiltinType::Kind::BFloat16:
return CGF.getContext().DoubleTy;
case BuiltinType::Kind::Double:
return CGF.getContext().LongDoubleTy;
default:
return ElementType;
}
}

QualType HigherPrecisionTypeForComplexArithmetic(QualType ElementType,
bool IsDivOpCode) {
QualType HigherElementType = GetHigherPrecisionFPType(ElementType);
const llvm::fltSemantics &ElementTypeSemantics =
CGF.getContext().getFloatTypeSemantics(ElementType);
const llvm::fltSemantics &HigherElementTypeSemantics =
CGF.getContext().getFloatTypeSemantics(HigherElementType);
if (llvm::APFloat::semanticsMaxExponent(ElementTypeSemantics) * 2 + 1 <=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd appreciate a comment here explaining why this is the correct check, so that future people reading this code can understand why this is being doing.

llvm::APFloat::semanticsMaxExponent(HigherElementTypeSemantics)) {
return CGF.getContext().getComplexType(HigherElementType);
} else {
FPHasBeenPromoted = LangOptions::ComplexRangeKind::CX_Improved;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm following correctly, this means that the behavior wouldn't be right for this code:

_Complex float func(_Complex float a, _Complex long double b, _Complex float c) {
  return (_Complex float)(b / c) / a;
}

The code would first process the long double complex division, which isn't promotable, and then the float division, using the same ComplexExprEmitter I think, so the resetting to improved would cause the second float division to be emitted as improved instead of promoted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I see the problem. Working on this. May be the FPHasBeenPromoted should be "attached" to the BinOpInfo instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see 2 different ComplexExprEmitter each with a different value of FPHasBeenPromoted though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is at line #2319 of CodeGen/cx-complex-range.c.

DiagnosticsEngine &Diags = CGF.CGM.getDiags();
Diags.Report(diag::warn_next_larger_fp_type_same_size_than_fp);
return CGF.getContext().getComplexType(ElementType);
}
}

QualType getPromotionType(QualType Ty, bool IsDivOpCode = false) {
if (auto *CT = Ty->getAs<ComplexType>()) {
QualType ElementType = CT->getElementType();
if (IsDivOpCode && ElementType->isFloatingType() &&
CGF.getLangOpts().getComplexRange() ==
LangOptions::ComplexRangeKind::CX_Promoted)
return HigherPrecisionTypeForComplexArithmetic(ElementType,
IsDivOpCode);
if (ElementType.UseExcessPrecision(CGF.getContext()))
return CGF.getContext().getComplexType(CGF.getContext().FloatTy);
}
Expand All @@ -300,11 +339,12 @@ class ComplexExprEmitter

#define HANDLEBINOP(OP) \
ComplexPairTy VisitBin##OP(const BinaryOperator *E) { \
QualType promotionTy = getPromotionType(E->getType()); \
QualType promotionTy = getPromotionType( \
E->getType(), \
(E->getOpcode() == BinaryOperatorKind::BO_Div) ? true : false); \
ComplexPairTy result = EmitBin##OP(EmitBinOps(E, promotionTy)); \
if (!promotionTy.isNull()) \
result = \
CGF.EmitUnPromotedValue(result, E->getType()); \
result = CGF.EmitUnPromotedValue(result, E->getType()); \
return result; \
}

Expand Down Expand Up @@ -794,8 +834,10 @@ ComplexPairTy ComplexExprEmitter::EmitBinMul(const BinOpInfo &Op) {
ResR = Builder.CreateFSub(AC, BD, "mul_r");
ResI = Builder.CreateFAdd(AD, BC, "mul_i");

if (Op.FPFeatures.getComplexRange() == LangOptions::CX_Limited ||
Op.FPFeatures.getComplexRange() == LangOptions::CX_Fortran)
if (Op.FPFeatures.getComplexRange() == LangOptions::CX_Basic ||
Op.FPFeatures.getComplexRange() == LangOptions::CX_Improved ||
Op.FPFeatures.getComplexRange() == LangOptions::CX_Promoted ||
CGF.getLangOpts().NoHonorInfs || CGF.getLangOpts().NoHonorNaNs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure NoHonorInfs or NoHonorNaNs should be checked here. Given that we have explicit control for complex arithmetic behavior, maybe that should take precedence. That seems to be the way gcc handles it: https://godbolt.org/z/1oGo7jznz

Copy link
Contributor Author

@zahiraam zahiraam Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is in EmitBinMul and your godbolt link has a div operation.
At any case the complex-range is not set when -ffinite-math-only is used on the command line. We are using the function applyFastMath to set the complex-range. It's called only when -ffast-math or -fp-model are used.
With CGF.getLangOpts().NoHonorInfs || CGF.getLangOpts().NoHonorNaNs we would get the same behavior than gcc for a mul operation: algebraic implementation with no nan testing.
If you don't want this condition here. then the complex-range needs to bet set when -ffinite-math-only is on the command line.
WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, for some reason I thought this was invoking the special handling for division. I think the for honor NaNs and infinities still isn't necessary because we'll emit the comparison with the fast-math flags set and the backend will optimize it away, which is what happens today: https://godbolt.org/z/e8EnKodj6

return ComplexPairTy(ResR, ResI);

// Emit the test for the real part becoming NaN and create a branch to
Expand Down Expand Up @@ -986,13 +1028,17 @@ ComplexPairTy ComplexExprEmitter::EmitBinDiv(const BinOpInfo &Op) {
llvm::Value *OrigLHSi = LHSi;
if (!LHSi)
LHSi = llvm::Constant::getNullValue(RHSi->getType());
if (Op.FPFeatures.getComplexRange() == LangOptions::CX_Fortran)
QualType ComplexElementTy = Op.Ty->castAs<ComplexType>()->getElementType();
if (Op.FPFeatures.getComplexRange() == LangOptions::CX_Improved ||
(Op.FPFeatures.getComplexRange() == LangOptions::CX_Promoted &&
FPHasBeenPromoted == LangOptions::CX_Improved))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this value is only ever set to CX_Improved or CX_None. Why not use a Boolean? As it is, I'm left with questions about what would happen if the value were CX_Basic or CX_None (expecting that CX_Promoted would be used if we did promote).

Copy link
Contributor Author

@zahiraam zahiraam Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think may be using a boolean for this variable makes sense. It's only set when there is type promotion. In which case the name of the variable is appropriate? May be an even better name is FPTypeHasBeenPromoted.

return EmitRangeReductionDiv(LHSr, LHSi, RHSr, RHSi);
else if (Op.FPFeatures.getComplexRange() == LangOptions::CX_Limited)
else if (Op.FPFeatures.getComplexRange() == LangOptions::CX_Basic ||
Op.FPFeatures.getComplexRange() == LangOptions::CX_Promoted)
return EmitAlgebraicDiv(LHSr, LHSi, RHSr, RHSi);
else if (!CGF.getLangOpts().FastMath ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove the fast-math check here. The driver handling of fast-math sets the complex arithmetic option. This check has always been problematic because disabling just one component of fast-math (such as enabling signed zeros) causes this to be false.

// '-ffast-math' is used in the command line but followed by an
// '-fno-cx-limited-range'.
// '-fno-cx-limited-range' or '-fcomplex-arithmetic=full'.
Op.FPFeatures.getComplexRange() == LangOptions::CX_Full) {
LHSi = OrigLHSi;
// If we have a complex operand on the RHS and FastMath is not allowed, we
Expand Down
Loading
Loading