-msse flag could be handled better by the driver

I bumped into two small issues with -msse and -msse4.2 flags:

1. GHC accepts any flag of the form -msseX.Y, eg. -msse3.14. I think we should validate numbers passed by the user or - even better - pre-define a set of allowed -msse flags. Currently we have this definition:

Flag "msse"         (versionSuffix (\maj min d -> d{ sseVersion = Just (maj, min) }))

1. A direct consequence of above definition is that bash command-line auto-completion does not suggest -msse4.2 flag.

This is somewhat related to Austin's D165

Trac metadata

Trac field	Value
Version	7.9
Type	Bug
TypeOfFailure	OtherFailure
Priority	normal
Resolution	Unresolved
Component	Driver
Test case
Differential revisions
BlockedBy
Related
Blocking
CC
Operating system
Architecture

changed weight to 5

added Tbug Trac import driver labels

I'd prefer an enumeration data type for enumerating the supported SSE versions; there aren't that many SSE versions

added newcomer label

assigned to @trac-dredozubov

Trac metadata

Trac field	Value
Related	→ #8404

I've added a simple type to restrict sse versions to major ones and 4.2(it's the only not *.0 version that've been explicitly checked in sources afaik). I'm not so sure if the naming is right on.

A direct consequence of above definition is that bash command-line auto-completion does not suggest -msse4.2 flag.

Probably it will work better with --show-options if separate flags will be used(such as -msse2/-msse4 etc).

Please advice and don't bash me too hard, it's my first attempt to work with ghc. :)

I commented on the Phab revision. That's a good start :-)

Oh, and when you upload a Phab revision you should fill in the appropriate field in the Trac ticket.

BTW. I'm surprised that the changes don't go outside of DynFlags.

Trac metadata

Trac field	Value
Differential revisions	→ D497

Thanks for your feedback!

I've updated the code to build on your suggestions. This way it'll show all possible flags with --show-options. I agree on "multiple -msse flags" point. It's completely reasonable to choose last version if multiple flags were provided by user. I feel like there's no need to leave VersionSuffix OptKind constructor(and code related to it), because there will be no application for it - it was commited and used only to support msse flags.

I'd gladly supply some tests to test flag-choosing behavior if i haven't been extremely frustrated how to do this. Any pointers on this matter will be greatly appreciated.

Trac metadata

Trac field	Value
Differential revisions	D497 → D504

I agree on "multiple -msse flags" point. It's completely reasonable to choose last version if multiple flags were provided by user.

What do you mean by "last version". "Latest SSE version" or "last version given on the command line"? I meant the former.

As for tests, you can test that:

1. all msse flags are listed by --show-options flag
2. incorrect -msse flag is rejected (see https://phabricator.haskell.org/D503 for an example how to do that).

additionally, im not sure if higher SSE numbers imply support for prior extensions, at least on non intel CPUS (in some cases it certainly does, but That maybe should be checked?)

jstolarek, I definitely meant "the latest SSE version". I'll try to look into writing tests for this patch tonight, sadly this week've been really busy and i was unable to work on something else except my day work.

1. all msse flags are listed by --show-options flag
2. incorrect -msse flag is rejected (see ​https://phabricator.haskell.org/D503 for an example how to do that).

This test case seems a little off to me. It tests OptKind + --show-options behaviour(which currently works), not patch in question. Maybe it'll be a little better to test that right flag(the latest sse version) would be set, if user provides two or more.

carter, afaik for each pair of versions of sse older one will contain subset of instructions from newer. I'm not really an expert on this matter though, so If someone else can verify that - it would be great.

hrm, you might be right. It seems to be true for intel and amd cpus at least. Question: how do clang and GCC handle this logic?

mentioned in commit da985920

closed

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

added Pnormal label