redo register assignment for SCfastpar and SCshadowreg parameters

[WalterBright]

Turns out the way this was done before was too clever, it would prevent allocation of register parameters to the registers they came in. This fix should result in better register allocation on 64 bit targets. It doesn't matter for 32 bit targets as parameters are passed on the stack, not registers.

[dlang-bot]

Thanks for your pull request, @WalterBright!

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9608"

[ibuclaw]

What is the point of this test if it's now disabled on all platforms dmd supports?

[WalterBright]

Because it's a highly annoying test.

1. It breaks every time I try to adjust the code generator.
2. Compiling the updating version of it is a nuisance.
3. Compiling the updating version requires the very latest Phobos to be installed and working.
4. Meanwhile, while trying to get (3) to work, the autotester won't test the rest of the compiler.
5. The compiler test suite shouldn't rely on Phobos, because that makes bootstrapping the compiler hard. Let alone the very latest Phobos (it requires std.typecons.EnumMembers).

I'll deal with it dead last.

[wilzbach]

the very latest Phobos to be installed and working.

No that test is almost two years old, so the update script can't rely on "the very latest Phobos".

The compiler test suite shouldn't rely on Phobos, because that makes bootstrapping the compiler hard

You do realize that the very script that you use to b execute the tests has been written in D? (d_do_tests)
Also, this test only requires Phobos for updating itself - not running.

[wilzbach]

DMD 2.060 (from 2012) has a bug fix for EnumMembers:

https://dlang.org/changelog/2.060.html

I don't have time to look more into the past and you probably still need a recent version of DMD/Phobos, because for example everything before 2.072 was compiled without PIC meaning that you can't run it on any modern Linux distribution. Also our bootstrapping version of DMD is 2.079. That's what all CIs use too. We're not going to cater for anything below 2.079.

[WalterBright]

The test updater bootstrap requires being compiled WITH THE COMPILER THAT IS UNDER TEST. This does not work, it has nothing to do with d_do_tests which can be compiled with an older build. Any change in the compiler that changes linking, like adding dip1000 changes, makes this NOT WORK.
test_cdcmp.d does not add value and is simply a nuisance.

Worse, it relies on all this working:

import std.algorithm : canFind, find, splitter, until;
import std.array : appender, join;
import std.conv : to;
import std.exception : enforce;
import std.file : readText;
import std.format : formattedWrite;
import std.meta : AliasSeq;
import std.path : baseName, setExtension;
import std.process : environment, execute, pipeProcess, wait;
import std.range : dropOne;
import std.regex : ctRegex, matchFirst, replaceFirstInto;
import std.stdio : File, stdout, writeln;
import std.string : strip;
import std.typecons : tuple, EnumMembers;

[WalterBright]

Basically all of Phobos has to work perfectly for this test to even compile, let alone run. It's just completely wrong for a back end test.

[andralex]

@wilzbach that dependency seems onerous.

[wilzbach]

Basically all of Phobos has to work perfectly for this test to even compile, let alone run.

NOPE: it requires no Phobos at all. Note the version(update):

dmd/test/runnable/test_cdcmp.d

Lines 26 to 28 in ecbb547

	version (update)
	{
	import std.algorithm : canFind, find, splitter, until;

It's only required for the automated update script.

[WalterBright]

The update version has to be run with the compiler under test, and it relies on Phobos.

[wilzbach]

Nope, you set the DMD under test via an environment (as the update script header explains) and can thus continue to use the host compiler for compiling the update script.

[wilzbach]

Let's not disable Martin's code gen tests without a good reason.

[WalterBright]

The "good reason" is because of a circular dependency on the compiler being tested to generate the tests, i.e. Phobos and the test updater have to be compiled with the compiler that is under test.

[WalterBright]

I'm planning some more alterations to the code generator after this one, which will just mess this up again. So I suggest deferring re-enabling these tests until the next release.

[wilzbach]

The "good reason" is because of a circular dependency on the compiler being tested to generate the tests, i.e. Phobos and the test updater have to be compiled with the compiler that is under test.

No, that's absolutely not true. You can use any moderately recent Phobos version (like e.g. the one bundled with the Host DMD that you have to use to compile DMD).

[WalterBright]

In this case, compiling with version=update fails because because of the change in function signatures due to the dip1000 changes.
The fundamental problem here is the compiler testsuite is supposed to NOT be dependent on Phobos. In this case, it is. There's no particular reason test_cdcmp.d must use Phobos. The test should remain disabled until it is fixed to not require Phobos.

[jacob-carlborg]

The test should remain disabled until it is fixed to not require Phobos.

Or it should be fixed before adding more code that depends on it instead of disabling it.

[WalterBright]

Or simply disable the test.

[andralex]

I'm okay with disabling the test. This way of validating compiler output is an ongoing job in and of itself.

[wilzbach]

I'm okay with disabling the test. This way of validating compiler output is an ongoing job in and of itself.

Sorry, this doesn't make any sense to me. The test in question has been added by Martin about two years ago. We have never had any problems with it so far and it's the only way we have to test this.
Disabling this is like saying we can't use D to run the testsuite, which we heavily use already
We can move the code to run these tests into d_do_test though which would also help to reduce the duplication.

Anyhow, as mentioned if we disable something that's been working well for years without any replacement, a better reasoning than principles is needed.

[WalterBright]

1. I've had trouble with it before, and updated it manually.
2. Again, it does not add value, it adds nuisance.
3. I haven't done much in the way of changing how the code generator works in the last couple years, except when I did resulting in (1).
4. I'm not deleting the test. Anyone can re-enable it in the future.

[wilzbach]

I've had trouble with it before, and updated it manually.

It comes with a handy auto-update feature. What's your issue with it?

Again, it does not add value, it adds nuisance.

I strongly disagree, see e.g. #7003 or #7849 for the motivation behind these tests.

I'm not deleting the test. Anyone can re-enable it in the future.

It's the same as deleting.

[WalterBright]

see e.g. #7003 or #7849 for the motivation behind these tests.

They're poor motivation. Even Martin wrote: "I've disabled the OSX and Windows asm comparison tests completely. They use the same backend code, but due to ABI differences the assembly is different. Seems more pragmatic to just disable those tests instead of inventing some fuzzy matching."

[wilzbach]

I've had trouble with it before, and updated it manually.

To clarify: I meant what's wrong with running the update script?

generated/linux/release/64/dmd -fPIC -version=update -run test/runnable/test_cdstrpar.d

It's easy and I just did that for you...
As you can see most tests gained one or two extra instructions, so I don't think that they are worthless.

If objdump is easier for you, I guess we can also convert these tests.

[wilzbach]

They're poor motivation. Even Martin wrote: "I've disabled the OSX and Windows asm comparison tests completely. They use the same backend code, [...]

@MartinNowak's point was that they use the same backend code which means that his added tests covered all platforms even if enabled only and thus avoided the need for fuzzy matching.

[MartinNowak]

Write a script that:

1. compiles test_cdcmp.d with -c -O

2. runs objdump on test_cdcmp.o and writes the output to test_cdcmp.out

3. diffs test_cdcmp.out with the known good output, test_cdcmp.txt

Sounds reasonable to me ;).

[WalterBright]

This change usually (but not always) produces better code, as the diffs show. The larger code generated examples are all due to the same effect - live register analysis is blocking using the fastpar register directly. I don't have a good solution to that at the moment, but this PR is still overall an improvement, and I don't want to make it even more complex.

Martin's code has been replaced with a shell script.

[WalterBright]

@wilzbach @thewilsonator this is ready now.

[andralex]

I hope the controversy has been put to sleep now. Thanks!

[ibuclaw]

This PR introduced a regression https://issues.dlang.org/show_bug.cgi?id=23084