No worries. I've been rebasing this series every time I've pulled mesa for the last few months, and this week is the first time I've had any real conflicts that need addressing. I'll see if I can find some time to address your comments and re-organize the commits as you suggested.
Jan also had some review comments previously that I need to go and confirm have all been addressed after I make changes. I've been pretty bad at finding time for clover/libclc recently (5.5 month old infant that refuses to sleep, who happens to have been born only 2 weeks after the previous iteration of this series was sent... coincidence? I think so). --Aaron On Fri, Feb 9, 2018 at 8:41 AM, Pierre Moreau <pierre.mor...@free.fr> wrote: > Hello Aaron, > > Sorry for not having reviewed the updated series… > I will have a look at it over the weekend. If I understand correctly, patches > 1 > and 2 have been squashed together and upstreamed already, while patches 3 > through 8 have not been merged yet. Is this series the latest version, or do > you have a more up-to-date version somewhere? > > These are just a few comments (before doing a real review over the weekend), > but I would agree with Jan’s comments of squashing some patches together. For > example, you could have: > * patches 5 and 6 squashed, and the result becomes the new patch 1, which > deals with passing the device down to the compiler; > * patches 4 and 7 squashed, and the result becomes the new patch 2, and > focuses > on introducing functions for handling the OpenCL version specified via > -cl-std: checking the value is valid, and converting the string to an > integer or a clang::LangStandard::Kind enum; > the last diff of patch 4 > - clang::LangStandard::lang_opencl11); > + get_language_version(opts, > device_version)); > is moved to the next patch instead; > * the new patch 3 takes care of dynamically setting the language version for > the compiler (taken from the old patch 4), and could be squashed with patch > 8 > as well. > * and at this point, I think the old patch 3 can be dropped as it is no longer > useful. > > Regards, > Pierre > > On 2017-07-30 — 20:26, Aaron Watry wrote: >> I've dropped the first patch of the previous series for now. I'm not >> withdrawing it completely, just going to see if there's anything about >> the user_ptr stuff that could have been causing the issue instead, and >> if I'm using too big a hammer in this patch. If I convince myself of its >> correctness, it'll be back. >> >> The rest of the patches move the device version declaration to core/device >> and then use that along with the -cl-std option to determine which >> OpenCL language version to enable in clang. >> >> I've done a full piglit run (again) before/after, and there are no changes >> for me on radeonsi/pitcairn if the device is left at CL 1.1. >> >> When I bump my platform/device versions to 1.2, the clang instance has >> been confirmed to enable 1.2 language features (like the static keyword >> required in test/cl/program/execute/static.cl, which goes skip->pass). >> >> Major changes since v1: >> Addressed Pierre's build-breakage comments >> Added a check for cl-std > device_clc_version >> Added a patch to pass the device object down into invocation.cpp >> instead of adding a bunch of device-based arguments. >> Use device_clc_version for cl version detection instead of device_version >> Added device_clc_version in device.cpp/hpp >> >> Anyway, happy reviewing. >> >> Cc: Jan Vesely <jan.ves...@rutgers.edu> >> Cc: Pierre Moreau <pierre.mor...@free.fr> >> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
