Skip to content

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Jan 8, 2018

This is first PR which works in the direction of finally going through with the
complex and imaginary type deprecation.

See also: dlang/dmd#7640

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

std/format.d Outdated
"float", "double", "real",
"char", "wchar", "dchar",
"ifloat", "idouble", "ireal",
"cfloat", "cdouble", "creal",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can we call typeid without a deprecation warning?

Copy link
Member

@ibuclaw ibuclaw Jan 8, 2018

Choose a reason for hiding this comment

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

I think the right steps are to first deprecate in library (user-code). Once enough time has passed, remove support for complex and imaginary in the library.

Then we kill it in the compiler starting with deprecation by default.

@wilzbach wilzbach changed the title Rework std.format Start to move away from complex and imaginary types Jan 8, 2018
@wilzbach wilzbach added the Review:WIP Work In Progress - not ready for review or pulling label Jan 8, 2018
assert(isIdentical(abs(-0.0L), 0.0L));
assert(isNaN(abs(real.nan)));
assert(abs(-real.infinity) == real.infinity);
assert(abs(-3.2Li) == 3.2L);
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 complex deprecation currently triggers error in deprecated code. See https://issues.dlang.org/show_bug.cgi?id=18212

@wilzbach
Copy link
Contributor Author

wilzbach commented Jan 8, 2018

I think the right steps are to first deprecate in library (user-code). Once enough time has passed, remove support for complex and imaginary in the library.
Then we kill it in the compiler starting with deprecation by default.

We can't do that. Phobos needs to build without deprecation messages. This was enabled about 9 months ago, because (1) deprecation messages were shown to users and (2) people always introduced deprecations without considering the fallout and viability. By -de we can enforce that if a symbol gets deprecated in Phobos, all usages of it have been removed (or are in deprecated blocks). Since it was introduced, it has proven as quite helfpul.

@andralex
Copy link
Member

andralex commented Jan 8, 2018

Phobos needs to build without deprecation messages.

Slight amendment: it's okay to have deprecation messages in phobos' unittest builds.

@wilzbach
Copy link
Contributor Author

wilzbach commented Jan 8, 2018

Slight amendment: it's okay to have deprecation messages in phobos' unittest builds.

Yep, but only if they are marked as deprecated. However, the complex deprecation messages doesn't respect this at the moment: https://issues.dlang.org/show_bug.cgi?id=18212

Copy link
Contributor Author

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Not so sure how to deal with functions returning creal. Would be adding deprecated creal opCast(T: creal) to std.complex good enough or do we have to completely deprecate these two functions?

std/math.d Outdated
{
return cos(y) + sin(y)*1i;
}
auto expi(real y) @trusted pure nothrow @nogc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to deal with this in another way.

std/math.d Outdated
* cos(z) = cos(z.re)*cosh(z.im) - sin(z.re)*sinh(z.im)i
*/
creal cos(creal z) @safe pure nothrow @nogc
auto cos(C)(C z) @safe pure nothrow @nogc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to deal with this in another way.

std/format.d Outdated
// is required since for arrays of ints we only have the mangled char
// to work from. If arrays always subclassed TypeInfo_Array this
// routine could go away.
private TypeInfo primitiveTypeInfo(Mangle m)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So primitiveTypeInfo is a relict from the old doFormat days. It's not used anywhere anymore.
See e.g. #4978

@wilzbach wilzbach changed the title Start to move away from complex and imaginary types [WIP] Start to move away from complex and imaginary types Jan 9, 2018
@9il
Copy link
Member

9il commented Jan 19, 2018

Do we need to move it to core? We can also test that certain modules in Phobos are -betterC, see e.g.

#5952

I do not think this is an issue. see #6014 (comment)

@thewilsonator
Copy link
Contributor

thewilsonator commented Jan 19, 2018

In practice we may need to mark function with one of other flags. But what flags should be choose core./std.complex? Should it be always fastmath? I should it be always not @fastmath? With native complex number we do not need to care about what kind of flags has library implementation. Native complex number inherit flags from the current function

Perhaps we should add an @inherit attribute for inline functions? Edit: or make the ldc.attributes template inheritable like the builtin attributes.

@9il
Copy link
Member

9il commented Jan 19, 2018

Perhaps we should add an @inherit attribute for inline functions?

Yes!!! But plus __traits(llvmAttributes) to chose algorithm depend on flags!
This would be awesome !

@9il
Copy link
Member

9il commented Jan 19, 2018

Perhaps we should add an @inherit attribute for inline functions?

Yes!!! But plus __traits(llvmAttributes) to chose algorithm depend on flags!
This would be awesome !

This will allow to solve all problems I have with std.math and std.complex. And I will be happy to remove mir.math.common then

@thewilsonator
Copy link
Contributor

But plus __traits(llvmAttributes) to chose algorithm depend on flags!

Please elaborate, I'm not sure what this would do.

@9il
Copy link
Member

9il commented Jan 19, 2018

But plus __traits(llvmAttributes) to chose algorithm depend on flags!

Please elaborate, I'm not sure what this would do.

__traits(llvmAttributes) returns an alias sequence of LLVM specific attributes that function has.

@fastmath auto foo(double a); { return bar(a); }
@inherit auto bar(double a) {
   // this trivial case can be done by LLVM optimizer,
   // but helpful for other tricky math hacks
    static if (AllowsReciprocal!(__traits(llvmAttributes)))
        return PI * 1 / a; // one algo
    else
        return PI / a; // another algo
}

template AllowsReciprocal(Attrs...) { ... }

@thewilsonator
Copy link
Contributor

That won't work because the body of bar must be independent of it's inlining into foo i.e. that static if has no knowledge of the inlining. You could template bar on a given set of attributes.

@9il
Copy link
Member

9il commented Jan 19, 2018

@thewilsonator can it be just a template without args? E. g. the attributes are special hiden template
arguments.

@wilzbach
Copy link
Contributor Author

We should be able to have almost full backwards compatibility

How would core.complex (i.e. -betterC complex) work?
std.complex

  • has nice formatting (requires std.format)
  • uses a lot of things from std.math which aren't -betterC

Wouldn't it then be just a copy of std.complex or in your words:

I would need to copy std.complex to mir, mark with appropriate LDC flags

@thewilsonator
Copy link
Contributor

Formating not so sure, any reason this can't be done one std.format as is done now?
Excluding the trig and ^^ and friends the only use of std.math is fabs.

@thewilsonator
Copy link
Contributor

@9il I don't think so.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 19, 2018

He means that std.complex is not annotated with LLVM optimisation hints (inlining, fast math). This leads to either duplication, or performance penalties and a dependance on phobos. Neither of which is desirable.

Neither are std.math or any other functions that use complex annotated with compiler specific attributes -> not a valid argument.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 19, 2018

Perhaps we should add an @inherit attribute for inline functions? Edit: or make the ldc.attributes template inheritable like the builtin attributes.

If it's not part of the language, then I can't see it going in. Likewise fastmath, which means that code generated is no longer IEEE compliant.

@9il
Copy link
Member

9il commented Jan 19, 2018

Neither are std.math or any other functions that use complex annotated with compiler specific attributes -> not a valid argument

There are no D libs for math and algorithms, but Phobos?

Entire mir-algorithm is annotated. I do not whant to argue why it is fine for IEEE, you can do experiments with LDC to ensure that IEEE is preserved if a lambda is not annotated.

@9il
Copy link
Member

9il commented Jan 19, 2018

If it's not part of the language, then I can't see it going in. Likewise fastmath, which means that code generated is no longer IEEE compliant.

It is part of the best D compiler for math and finance.

@stefan-koch-sociomantic

@9il AllowsReciprocal ?

when would that ever not be allowed ?
b/c of precision issues ?

@9il
Copy link
Member

9il commented Jan 19, 2018

@stefan-koch-sociomantic div operation can be optimised in fastmath to mul and reciprocal. This optimization is not allowed by default because of IEEE

@ibuclaw
Copy link
Member

ibuclaw commented Jan 19, 2018

Entire mir-algorithm is annotated. I do not whant to argue why it is fine for IEEE

You're free to do that if you value speed over correctness.

Talking about annotating functions with non-standard attributes is not up for debate though.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 19, 2018

It is part of the best D compiler for math and finance.

The best compiler for math and finance is gdc (I say that with a grin, but the point stands - there is no best).

(incidentally one of the Berlin meet up talks did a live demo talk benchmarking math performance between dmd, ldc, gdc - the winner was gdc by a landslide, the loser was ldc but the author was surprised and didn't know why).

@stefan-koch-sociomantic
Copy link

stefan-koch-sociomantic commented Jan 19, 2018

gdc's frontend does a better job then ldc's does.
also the gcc optimizers are capable of performing rewrites in more cases.
llvm's development is mostly driven by clang which emits rather specific patterns.
and in the optimizers only those patterns get recognized.

@9il
Copy link
Member

9il commented Jan 19, 2018

@ibuclaw this is your opinion. Mir-algorithm does not break IEEE, probably you do not understand how this attributes affects lambdas.

I do not debate. I just showed how the prs kill perfomance. I will be happy with my own library

@ibuclaw
Copy link
Member

ibuclaw commented Jan 19, 2018

@9il I'll start by removing fastmath from your examples and seeing what the real difference is.

@thewilsonator
Copy link
Contributor

thewilsonator commented Jan 19, 2018

Perhaps we should add an @inherit attribute for inline functions? Edit: or make the ldc.attributes template inheritable like the builtin attributes.

If it's not part of the language, then I can't see it going in. Likewise fastmath, which means that code generated is no longer IEEE compliant.

@ibuclaw Quite the opposite.@inherit has nothing to do directly with IEEE conformance except propagating attributes of a function to functions inlined into it e.g.

@inherit void bar(...) { ... } // <-not @fastmath
@fastmath void foo(...)
{
   bar(...); // inlined into foo. foo's inline copy of bar now @fastmath
}

If there becomes a core.complex (and probably even if it stays as std.complex) LDC's implementation may add that attribute.

The question is if there is benefit in standardising it across compilers for propagating optimisation directives.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 19, 2018

The question is if there is benefit in standardising it across compilers for propagating optimisation directives.

You could start with a DIP. ☺️

@thewilsonator
Copy link
Contributor

Well would GDC benefit from this? DMD doesn't have much in the way of perf tweaking attributes.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 19, 2018

Try playing around with gdc yourself, and see if there might be: https://explore.dgnu.org/

The module you're looking for is gcc.attribute, and the uda you should test is @attribute("optimize", "-fsome-optimizer-flag").

From what you describe though, I'm not sure. I would expect all inlined functions to be applied the same optimization flags as the function they are inlined into without the need to be explicit about it.

@thewilsonator
Copy link
Contributor

Well in LDC the IEEE compliance (or lack thereof) is encoded into the function such that inlining will result in some instructions being @fastmath and some not. Maybe it is done differently in GCC?

@ibuclaw
Copy link
Member

ibuclaw commented Jan 19, 2018

In any case. Any solution or discussion needs to revolve around compiler-agnostic tests for performance and attributes. Anything else is out of scope.

We could introduce core.complex, but for what purpose? There is no complex in the language. So it will end up being a copy of std.complex.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 19, 2018

Well in LDC the IEEE compliance (or lack thereof) is encoded into the function such that inlining will result in some instructions being @fastmath and some not. Maybe it is done differently in GCC?

I couldn't say off the top of my head. If anything it probably depends entirely on what order optimization passes are done in. I.e: whether or not inlining is done before or after reordering.

I'm pretty certain that optimisation attributes are applied only at a function level and not a block level. So I don't think ldc's problem really applies for gdc. But as I said please test this! (I'm on my phone and can't for the moment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants