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.

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

Issue 8425 - Missing line number and module name that can't use core.simd #2879

Merged
1 commit merged into from Sep 7, 2014

Conversation

yebblies
Copy link
Member

Fix the line number, fix is(__vector(XXX)), fix -o-

@ibuclaw @redstar @klickverbot

How should I fix it so it works for compilers that support more than x86?
Do you need more fine-grained support variables or what?

https://d.puremagic.com/issues/show_bug.cgi?id=8425

@ibuclaw
Copy link
Member

ibuclaw commented Nov 25, 2013

GCC has a vector_mode_supported_p() target hook that can use to evaluate vectors on a per-case basis at the point of declaration. I had planned to implement a target hook to do just that for dmd instead of the type size checking done in mtype.c.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 25, 2013

Lazy man's implementation (can be done much better)

/***********************************
 * Return true if the target has hardware support for vector 'type'
 */
bool Target::hasvectortype(Loc loc, TypeVector* type)
{
    assert(type->basetype->ty == Tsarray);

    TypeSArray *t = (TypeSArray *)type->basetype;
    d_uns64 sz = t->size(loc);
    if (sz != 16 && sz != 32)
    {
        error(loc, "base type of __vector must be a 16 or 32 byte static array, not %s", t->toChars());
        return false;
    }
    if (!global.params.is64bit && !global.params.isOSX)
    {
        error(loc, "SIMD vector types not supported on this platform");
        return false;
    }   
    if (sz == 32)
    {
        error(loc, "AVX vector types not supported");
        return false;
    }

    // Hardware support, woo hoo!
    return true;
}

@dnadlinger
Copy link
Member

The question is: What does D_SIMD actually mean when individual types can not be available? Maybe that's even in the spec but I just forgot, though.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 25, 2013

I thought D_SIMD meant that __simd is available (that's what I agreed with when I raised my concerns with core.simd, that is what I last recall reading in the spec).

@yebblies
Copy link
Member Author

Do we really want to be emitting errors from Target?

@ibuclaw
Copy link
Member

ibuclaw commented Nov 26, 2013

No we don't. :)

Ideally those three errors should be squashed into one if possible.

@yebblies
Copy link
Member Author

How about this? Multiple, specific errors, determined by Target.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 26, 2013

We can do that, now I'm just not sure about your test in testxmm.d

See:
dlang/druntime@44001eb

and D_SIMD definition:
http://dlang.org/version.html

@yebblies
Copy link
Member Author

I don't understand?

@ibuclaw
Copy link
Member

ibuclaw commented Nov 26, 2013

Doesn't static assert(!is(__vector(void[16]))); mean that you are asserting that using __vector(void[16]) should throw an error if !D_SIMD ?

@yebblies
Copy link
Member Author

Yes. Is that a problem?

@ibuclaw
Copy link
Member

ibuclaw commented Nov 26, 2013

The point of D_SIMD was that it separated DMD's __simd() hack from LDC and GDC's existing backend support.

Evidently the spec needs more clarification. ;-)

@yebblies
Copy link
Member Author

Hmm ok. How do you tell if __vector() types are valid then? (before this pull)

@ibuclaw
Copy link
Member

ibuclaw commented Nov 27, 2013

Hmm... the problem is that is an over generalised question. There's no version condition for each possible __vector type/size combination that the target could support, so you will have to go off static if (is(__vector(T)))

@ibuclaw
Copy link
Member

ibuclaw commented Nov 27, 2013

You could however make assumptions on version(ARCH) - but for gdc at least, that could be a flawed assumption if eg: you are X86 but the compiler built is for targetting i386 (no MMX, SSE, AVX).

@yebblies
Copy link
Member Author

Ok, that's what I was thinking. But in that case - what should I use to version the aliases in core.simd? (In a way that will be non-dmd compatible)

@ibuclaw
Copy link
Member

ibuclaw commented Nov 27, 2013

That is a good question - since I implemented vector support (literally hours after Walter added front-end support) I have not cared about hardware support, as gcc can safely emulate most vectors in the most natural way for the target.

@yebblies
Copy link
Member Author

This is tricky. Do you add float4 style aliases for other vector types that dmd doesn't support? Or is core.simd used as-is? I'm not sure what assumptions to make about druntime and gdc/ldc.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 27, 2013

I believe I said this during my talk, I accidentally committed support for 8 and 32-byte vectors before at least 32-byte vectors were supported by dmd.

D-Programming-GDC/gdc@018fb6d#diff-8f1c174f0ba2889fb08f2f7e8a287f4aL3435

Someone noticed and it's been left this way ever since.

D-Programming-GDC/gdc@3d447d9

DMD doesn't support the 8 byte vectors, and has limited support for 32 byte vectors. But I don't think it will be a problem leaving it as is, the compiler (should) only error if you actually try to use the type.

@yebblies
Copy link
Member Author

No, the compiler will error as soon as it runs semantic on the alias.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 27, 2013

then how does the dmd get away with defining 32 byte aliases in core.simd and not running into problems for non-supported platforms?

@ibuclaw
Copy link
Member

ibuclaw commented Nov 27, 2013

unless of course it doesn't try to compile core.simd... :o)

@yebblies
Copy link
Member Author

Because currently, the error is in the glue layer. This pull moves it into semantic.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 27, 2013

ah ha hmm... Maybe go down door number three and request that lack of hardware support be downgraded to a warning?

@ibuclaw
Copy link
Member

ibuclaw commented Nov 27, 2013

This would be:

case 4:
   warning(loc, "blah");
   break;

Also, we need an enum for these numbers

@yebblies
Copy link
Member Author

I don't think a warning will do it. The compiler need to tell you "don't use this type, I can't generate code for it".

Heh, could always do
static if (is(typeof(__vector(int[4])))) alias int4 = __vector(int[4]);
x30

@ibuclaw
Copy link
Member

ibuclaw commented Nov 27, 2013

That is what case 1: or case 2: is for. DMD itself does not have a case to use warnings in this instance.

@yebblies
Copy link
Member Author

I don't see how that helps anything.

@redstar
Copy link
Contributor

redstar commented Nov 27, 2013

With LDC, vector types are always valid. If there is no hardware support then the operations are simulated. I know that this is somewhat against the spirit of the vector types in D. But there is no real way to find out which backend supports which instructions.

@yebblies
Copy link
Member Author

I'm leaning toward just doing static if (is(typeof(X)) alias floatN = X; So, automatically declare aliases if the type is available. The downside is this turns "vector type not supported" into "unknown identifier float8".

@yebblies
Copy link
Member Author

yebblies commented Feb 8, 2014

Doesn't work, maybe can't work.

It's trying to move a glue error into the frontend, which would be fine, except this error is really just a limitation of the dmd backend and not a real language error.

I'm hoping a solution will come to me, but it doesn't seem to be working.

@andralex
Copy link
Member

redping

@yebblies
Copy link
Member Author

Hmm, if @disable took an error message this could be done quite easily without ruining the error messages.

@ghost
Copy link

ghost commented May 17, 2014

Someone needs to get on Issue 8728.

@TurkeyMan
Copy link
Contributor

I agree, the preference would be static if (is(__vector(float[4]))) alias float4 = __vector(float[4]);, but the problem is that this obscures the error message right? That's where we're at with this?

For clarification, is it correct that D_SIMD refers only to __simd (as I understand it?). That should be clarified in the spec...
The following sentence in the spec states though that Whether a type exists or not can be tested at compile time with an IsExpression:, which is as I've always understood it, and I think that's correct. I think we need to make a documented commitment to D_SIMD means __simd.

@ibuclaw @klickverbot From the bottom of the spec, there is a bunch of info supposedly for x86 and x64 showing supported types, and also supported operations on those types.
Do you guys think it's reasonable to present a table like that? I guess it must have come from one of you, since those types and operations aren't supported by DMD.
The problem is, it doesn't reflect the complex nature of SSE versions. It just seems to assume AVX2 is the gospel, which shouldn't be the default for many years to come yet... I think those tables could be dangerously misused, and are really quite inaccurate conceptually.

@yebblies
Copy link
Member Author

the problem is that this obscures the error message right? That's where we're at with this?

Yeah. I'm leaning towards just doing it and letting the error message suck. You only see it if you use float4, __vector(float[4]) will give the correct error.

@TurkeyMan
Copy link
Contributor

I just tried something like this:

template float4(T = int) // default template arg
{
  static assert(0, "SIMD not supported error"); // mechanism to produce error message
  alias float4 = T; // type that can be instantiated, to keep further semantic analysis happy.
}

But I was surprised to find that:

float4 x;

doesn't work. It complains Error: float4(T = int) is used as a type. It expects:

float4!() x;

Why doesn't the default template arg allow omission of the template argument list?

@yebblies
Copy link
Member Author

Why doesn't the default template arg allow omission of the template argument list?

Because then there's no way to refer to the template symbol. It would be quite useful in this case though.

@TurkeyMan
Copy link
Contributor

OT: That's the reason? Hmm, surely that's surmountable... what are some common uses to reference the template symbol rather than an instantiation? I can't imagine very many uses for that case, perhaps that should require some special syntax to be explicit, given it's the rare case?

@yebblies
Copy link
Member Author

dlang/druntime#795 will fix the druntime failure

@yebblies
Copy link
Member Author

yebblies commented Sep 7, 2014

Needs dlang/phobos#2490

@yebblies yebblies force-pushed the issue8425 branch 3 times, most recently from c4917c5 to f70159a Compare September 7, 2014 19:43
@yebblies
Copy link
Member Author

yebblies commented Sep 7, 2014

Finally green!

@ghost
Copy link

ghost commented Sep 7, 2014

Auto-merge toggled on

ghost pushed a commit that referenced this pull request Sep 7, 2014
Issue 8425 - Missing line number and module name that can't use core.simd
@ghost ghost merged commit cd5a060 into dlang:master Sep 7, 2014
@ghost
Copy link

ghost commented Sep 7, 2014

Great work @yebblies!

dnadlinger added a commit to dnadlinger/dmd that referenced this pull request Sep 8, 2014
@dnadlinger
Copy link
Member

Why all lower-case and the magic integer constants instead of an enum?

@yebblies yebblies deleted the issue8425 branch September 8, 2014 06:58
@yebblies
Copy link
Member Author

yebblies commented Sep 8, 2014

@AndrejMitrovic Thanks!
@klickverbot I dunno, I made this ages ago.

@ibuclaw
Copy link
Member

ibuclaw commented Sep 15, 2014

@klickverbot I dunno, I made this ages ago.

Could always raise a new pull to fix that.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants