Skip to content
This repository was archived by the owner on Jun 20, 2019. It is now read-only.

Fix assertion failure in datetime.stopwatch when compiling with -O2#639

Merged
ibuclaw merged 1 commit intoD-Programming-GDC:masterfrom
ibuclaw:stopwatchopt
Mar 11, 2018
Merged

Fix assertion failure in datetime.stopwatch when compiling with -O2#639
ibuclaw merged 1 commit intoD-Programming-GDC:masterfrom
ibuclaw:stopwatchopt

Conversation

@ibuclaw
Copy link
Copy Markdown
Member

@ibuclaw ibuclaw commented Mar 11, 2018

The benchmark could take no time at all because the benchmark is inlined, and the loop is removed.

@PetarKirov
Copy link
Copy Markdown

PetarKirov commented Mar 11, 2018

@ibuclaw can you help forge upstream an intrinsic (or a set of) for use in std.benchmark which would prevent similar compiler optimizations? Would the GDC syntax equivalent of the inline assembly shown in the CppCon 2015 talk https://stackoverflow.com/questions/33975479/escape-and-clobber-equivalent-in-msvc Just Work tm?

@ibuclaw
Copy link
Copy Markdown
Member Author

ibuclaw commented Mar 11, 2018

In this case its not so much a problem that the first benchmark takes no time, the unittest here is only making sure that the second function took longer.

@jpf91
Copy link
Copy Markdown
Contributor

jpf91 commented Mar 11, 2018

For this specific case it probably doesn't matter, but I think std.benchmark should prevent such optimizations in general. So we should have a look at the solutions presented in that talk.

Copy link
Copy Markdown
Contributor

@jpf91 jpf91 left a comment

Choose a reason for hiding this comment

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

OK, but why version(GNU)? LDC could be affected as well, or maybe will be in the future.

@ibuclaw
Copy link
Copy Markdown
Member Author

ibuclaw commented Mar 11, 2018

@jpf91 - Oh, I'm just going to push the >= condition upstream, so when we (eventually) merge it in, the version condition can be removed.

Edit, actually, I was beaten. Upstream PR is here: dlang/phobos#6265

@ibuclaw
Copy link
Copy Markdown
Member Author

ibuclaw commented Mar 11, 2018

@ZombineDev - As for the assembler, it should work to have:

void escape(void* p)
{
    asm { "" : : "g"(p) : "memory"; }
}

void clobber()
{
    asm { "" : : : "memory"; }
}

All asm statements are marked volatile, unless they are asm pure { }.

@ibuclaw
Copy link
Copy Markdown
Member Author

ibuclaw commented Mar 11, 2018

Granted though, I don't think there's anything peculiar about these primitives that they can't just be intrinsics also.

@ibuclaw ibuclaw merged commit 90fb72b into D-Programming-GDC:master Mar 11, 2018
@ibuclaw ibuclaw deleted the stopwatchopt branch March 11, 2018 21:54
@wilzbach
Copy link
Copy Markdown

For this specific case it probably doesn't matter, but I think std.benchmark should prevent such optimizations in general.

See also: dlang/phobos#5416

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants