These are chat archives for libmir/public

26th
Sep 2016
Nicholas Wilson
@thewilsonator
Sep 26 2016 03:42
speaking of which ldc-developers/ldc#1786
Ilya
@9il
Sep 26 2016 05:35
Awesome, thank you!
Nicholas Wilson
@thewilsonator
Sep 26 2016 05:36
Have you managed to build it?
Ilya
@9il
Sep 26 2016 05:37
Have not tried yet
Looks like it is failing CI tests
Nicholas Wilson
@thewilsonator
Sep 26 2016 05:38
Let me know if you have problems
Ilya
@9il
Sep 26 2016 05:38
ok, thanks
Nicholas Wilson
@thewilsonator
Sep 26 2016 05:38
Yeah ,circle is trying to use llvm4.0 which it can't find
Appveyor is missing _d_allocclass, idk why. Perhaps a druntime version mismatch.
Nicholas Wilson
@thewilsonator
Sep 26 2016 06:13
I think I mis-resolved the druntime subproject commits during the larger rebase. I hope it is fixed now.
Nicholas Wilson
@thewilsonator
Sep 26 2016 08:18
Lights are green, well kinda. Travis with 3.5.2 failed, but I thought support for 3.5 was/is going to be dropped. Circle still can't find 4.0. Appveyor passes.
Johan Engelen
@JohanEngelen
Sep 26 2016 09:40
@thewilsonator Of course the dcompute work is very exciting. But your PR is massive. For merging... you should split the work into smaller pieces, and submit PRs for those. Some of your changes are simple, like "alloc point"-->"allocPoint", and we can easily merge those in a separate PR. So that they don't distract from the actual work in the PR, making review 100000x easier. For all the if(!dct) PGOstuff, you should modify the PGO code itself, and have it check whether to emit PGO code or not. For example, there is the emitInstrumentation variable, but perhaps it is good to add another disable/enable flag in there, that is then set by the // Initialize PGO state for this function code in gen/functions.cpp, such that you can easily disable it for all dcompute code without having the explicit if-statements everywhere in the code.
@thewilsonator Most importantly: there are NO TESTS in the PR !!!!!!!!!!!!!
@thewilsonator And Ilya's questions in the PR are very much about the PR. The dcompute work needs careful thought before adding it. Coding of the feature is one thing, but it needs to be thought through from a user perspective aswell. You know I had my doubts too about the "per module" approach that you took.
Nicholas Wilson
@thewilsonator
Sep 26 2016 09:43
Re the size it was +3000,-100 or there a bouts ;)
Johan Engelen
@JohanEngelen
Sep 26 2016 09:44
It is a very pervasive change.
Nicholas Wilson
@thewilsonator
Sep 26 2016 09:44
re if(!dct) PGO yes
Johan Engelen
@JohanEngelen
Sep 26 2016 09:44
Please read this document: http://llvm.org/docs/DeveloperPolicy.html
Because the change is so large, some bad things are easily missed. Like this one:
-    args.push_back(p);
 +    llvm::SmallString<24> s(p);
 +    if (s.endswith(".spv") || s.endswith(".ptx")) {
 +      continue;
 +    }
 +    args.push_back(std::string(p));
why is there suddenly a std::string(p) needed?
-      Module *const m = modules[i];
 +      Module * m = modules[i];
why is the const removed? Bad!
because it is not something that is just done and we never have to look at it again, we need to make sure that don't end up with too many hacks in the code that will not be fixed any more and are not understandable by other devs. (there is already too much of that in LDC)
Johan Engelen
@JohanEngelen
Sep 26 2016 09:50
now that you have much more understanding of LDC's internals and the LLVM backend for dcompute, I think you should step back and discuss with people about what the D code should look like (not at all thinking about LDC implementation details), what capabilities people want to have. And then start a new branch, and copy pieces from your current dcompute branch and "reimplement" things with the knowledge you gained.
Nicholas Wilson
@thewilsonator
Sep 26 2016 09:51
Re const I think I was trying to fix the -singleobj segfault but didn't get far.
Ilya
@9il
Sep 26 2016 17:16
100 github stars :smile: https://github.com/libmir/mir
Johan Engelen
@JohanEngelen
Sep 26 2016 18:22
nice Ilya :-)