[Mesa-dev] [PATCH 0/8 v2] A few clover fixes for both CTS and eventual 1.2 support

[Pierre Moreau]

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.vesely at rutgers.edu>
> Cc: Pierre Moreau <pierre.morrow at free.fr>
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180209/360d2478/attachment.sig>