This patch breaks formatting. If there's a spaces after opening "(" there should be one before closing ")"

In D107849#2939767, @gAlfonso-bit wrote:

In D107849#2937885, @arsenm wrote:

I don't understand what you are trying to fix. What crashes exactly?

It doesn’t crash anymore, so we can remove the workarounds in place. That’s the point.

LGTM.

Was this tested on an existing device, or is it just for completion (since there already is clspv target)?

ping, @tstellar any concerns about this one?

ping

In D99794#2673632, @Anastasia wrote:

In D99794#2672235, @jvesely wrote:

In D99794#2671843, @Anastasia wrote:

Btw just out of curiosity are there any reasons for libclc not to use clang implicit headers? They support all OpenCL standards and get a lot of new features and bug fixes regularly.

historic. libclc was ignored by most of the clang opencl work and nobody volunteered to switch it to clang provided headers.
Moreover, it has been stuck in clc 1.1/1.2 so most of the new features don't apply.
The main user is mesa/clover so that one needs to be switched to using clang implicit headers first

OK, I see. Thanks for the clarification! Btw the functionality of version 1.1. and 1.2 is fairly complete in clang headers. So it should be relatively low risk if you switch to it. If there are any specific adjustments needed for libclc we can surely find a way to accommodate those.

In D99794#2671843, @Anastasia wrote:

Btw just out of curiosity are there any reasons for libclc not to use clang implicit headers? They support all OpenCL standards and get a lot of new features and bug fixes regularly.

In D94013#2600823, @alan-baker wrote:

I pushed https://github.com/alan-baker/llvm-project/commit/248f6480fecb04c343da06aee437d27d75b3a9f6 which converts tabs to spaces in fma.cl if that is helpful.

In D94013#2593369, @alan-baker wrote:

I've pushed the commit to my fork at 30d41a. I just rebased it on the latest main.

it took a while to test both versions. this patch fixes the following conversion tests:

	 *** 622) convert_floatn_rtp( uintn ) FAILED ** 
	 *** 627) convert_floatn_rtp( intn ) FAILED ** 
	 *** 639) convert_floatn_rtz( doublen ) FAILED ** 
	 *** 649) convert_floatn_rtz( longn ) FAILED ** 
	 *** 689) convert_doublen_rtz( doublen ) FAILED **

The number of failed cases goes down from 42 to 37

In D94013#2585814, @alan-baker wrote:

Thanks for the review! Would it be possible for someone with commit privileges to land this patch?

thanks

while the fix is clearly correct, it'd be nice to do a test run and report changes.
I can run it on my carrizo machine, but the conversion tests take more than a day.

In D94013#2581830, @alan-baker wrote:

Is there anything else necessary to complete this review?

thanks. can you fix the "to avoid to use" part of the description? the "to avoid" part sounds redundant.
It explains why fma needs to be different, but it could use an explanation why it needs sw implementation of nextafter.

In D94013#2566025, @alan-baker wrote:

In D94013#2562727, @jvesely wrote:

The FMA does pass conformance. I don't have performance numbers for the 64-bit version vs the uint2 version. It would be worse, but the whole software FMA is already awful performance. The 64-bit version is simpler to understand, so unless you have the same restrictions as I need for clspv, then I wouldn't adopt it.

Hopefully, the different target name will clarify the intent better. This is targeted for use with a cross compiler to Vulkan SPIR-V. The needs are different than the spir targets.

There are existing spirv and spirv64 targets. The new clspv target uses the same spir-- triple as the spirv target, but doesn't use the final spv conversion step and uses O3 optimization. The selection of files is mostly a subset of the existing spirv target.

Is there somewhere in the repo you'd suggest I document the differences? Nothing jumped out at me that explains the various backends. In addition to fma, nextafter is also a different implementation than the spirv targets.

The FMA does pass conformance. I don't have performance numbers for the 64-bit version vs the uint2 version. It would be worse, but the whole software FMA is already awful performance. The 64-bit version is simpler to understand, so unless you have the same restrictions as I need for clspv, then I wouldn't adopt it.

Hopefully, the different target name will clarify the intent better. This is targeted for use with a cross compiler to Vulkan SPIR-V. The needs are different than the spir targets.

It'd be nice to add an explanation why this can't reuse the existing SPIR-V targets and needs to add a new one.
Does the new FMA implementation generate significantly worse code (EG/NI + mesa-spirv should be the only users) than the original? Should it just be replaced? I assume it passes the conformance suite.

In D89372#2335027, @Anastasia wrote:

In D89372#2334728, @jvesely wrote:

In D89372#2334225, @Anastasia wrote:

Ok, so the pragma is supposed to control the pointer dereferencing which seems like a valid case but however, it is not implemented now. Do we know of any immediate plans from anyone to implement this? If not I would prefer to remove the pragma for now? If someone decided to implement this functionality later fully it is absolutely fine. Pragma will be very easy to add. There is no need for everyone to pay the price for something that can't be used at the moment.

I though the current behaviour of 'you can use #pragma, but the dereferences are always legal' was intentional for backward compatibility.
I don't think there are programs that would set it to 'disabled' and expect compilation failure.

The initial state of the pragma is disabled, so if disabling the extension isn't supposed to do anything then I don't know why would anyone ever enable it?

My concern is that legacy apps would set '#pragma enabled' before using char/short. such behavior would produce warning/error if with this change applied.

Correct, it will compile with a warning but not fail to compile. I am willing to introduce a -W option (if not present already ) to suppress that warning if it helps with the clean up and backward compatibility. It might also open up opportunities for a wider clean up - removing pragma in extensions that currently require pragma for no good reason. I have written more details on this in https://github.com/KhronosGroup/OpenCL-Docs/pull/355#issuecomment-662679499

In D89372#2334225, @Anastasia wrote:

Ok, so the pragma is supposed to control the pointer dereferencing which seems like a valid case but however, it is not implemented now. Do we know of any immediate plans from anyone to implement this? If not I would prefer to remove the pragma for now? If someone decided to implement this functionality later fully it is absolutely fine. Pragma will be very easy to add. There is no need for everyone to pay the price for something that can't be used at the moment.

In D89372#2332997, @Anastasia wrote:

In D89372#2332853, @jvesely wrote:

cl_khr_byte_addressable_stores changes language semantics. without it, pointer dereferences of types smaller than 32 bits are illegal.

Ok, does it mean that we are missing to diagnose this in the frontend? Btw I do acknowledge that what you say makes sense but I don't think the documentation support that:
https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_byte_addressable_store.html

Am I just looking in the wrong place?

cl_khr_byte_addressable_stores changes language semantics. without it, pointer dereferences of types smaller than 32 bits are illegal.
Even if all clang targets support this the macro should still be defined for backward compatibility.

LGTM

I think this should use FindPython3 and drop the shebang from gen_convert.py to fix the 2 vs. 3 issues as well.

I think these routines need to be rewritten and tested with CTS on platforms that use them.

In D85911#2264302, @jenatali wrote:

@jvesely, would you be able to land this for us if you're happy with it?

In D85911#2250154, @daniels wrote:

In D85911#2250140, @jvesely wrote:

so the translator can translate calls to fma, but not call to llvm.fma? that's weird. Please add that in a comment. Otherwise LGTM.

Right, it doesn't translate the LLVM intrinsics, only what's covered by the spec (see e.g. the OpenCL extension opcode definitions) as builtins, so it can be precise about what they mean and require.

Would you like the comment in fma.inc or in a commit message?

In D85911#2247585, @jenatali wrote:

In D85911#2247580, @jvesely wrote:

I understand the mechanism you're aiming for. It' not clear whether the absence of fp64 path is intentional or not. afaik, all hw exposing fp64 also implements fma.fp64 in hw so generic clc implementation works.

Agreed, I expect all fp64 hardware has a native fp64 fma. However, for hardware-native implementations of CL functionality, we don't need a libclc function which maps to an LLVM intrinsic, because the SPIRV-LLVM-Translator maps calls to fma() to a SPIR-V opcode -- regardless of whether that fma() has an implementation present in the module. In fact, we *can't* have functions which map to LLVM intrinsics, because the SPIRV-LLVM-Translator will *fail* to map that to SPIR-V at all -- it doesn't map to the SPIR-V opcode.

resuing the generic clc fma.cl still works to expose the knob for fma.fp32 added in D85910

It does not, because that results in code which calls the LLVM intrinsic, which fails to translate to SPIR-V.

I understand the mechanism you're aiming for. It' not clear whether the absence of fp64 path is intentional or not. afaik, all hw exposing fp64 also implements fma.fp64 in hw so generic clc implementation works.

Why does this need a special implementation of fma.cl and fma.inc? The fp64 paths are missing, is there an expectation that something else will translate fma(double) function calls to llvm.fma opcodes?

Thanks. This LGTM, but you might want to get @tstellar's opinion as well.

Have you checked that returning NULL is handled properly?
Should the buffer wrap around instead?

In D84392#2170909, @jvesely wrote:

In D84392#2170733, @arsenm wrote:

In D84392#2170732, @arsenm wrote:

In D84392#2170731, @jvesely wrote:

Does the printf expansion work for all amdgpu triples, including amdgcn--mesa3d?
will it work for non-ROC capable GPUs?

Nothing in the compiler actually depends on roc capable GPUs. The limitations are all runtime/driver related

The exception to this would be places in the libraries that assume flat instructions are available. Theoretically we could handle these in the backend for SI with tagged pointers or something

so there are no hidden assumptions; like the printf buffer being cache coherent with host CPU, or the implementation using s_sendmsg to trigger CPU draining the buffer before the kernel finishes execution?

The implementation use a regular device buffer that is read at the end of the execution. Mostly like a clEnqueueReadBuffer

In D84392#2170733, @arsenm wrote:

In D84392#2170732, @arsenm wrote:

In D84392#2170731, @jvesely wrote:

Does the printf expansion work for all amdgpu triples, including amdgcn--mesa3d?
will it work for non-ROC capable GPUs?

Nothing in the compiler actually depends on roc capable GPUs. The limitations are all runtime/driver related

The exception to this would be places in the libraries that assume flat instructions are available. Theoretically we could handle these in the backend for SI with tagged pointers or something

Does the printf expansion work for all amdgpu triples, including amdgcn--mesa3d?
will it work for non-ROC capable GPUs?

I don't think there are many benefits in merging this ahead of time without the rest of printf implementation so that it can be tested.

In D83473#2145805, @bbrezillon wrote:

Since OpenCL allows apps to provide precompiled intermediate representations to the API, in the form of SPIR or SPIR-V, that means that the app could have embedded references to FP_ILOGBNAN, which are just a constant value since it's a #define, in their own code. They could write a kernel which calls ilogb and compares the result to FP_ILOGBNAN, and compile that with either clang trunk (which uses opencl-c.h automatically), or with Khronos's offline compiler (https://github.com/KhronosGroup/SPIR) using Khronos's standard library headers (https://github.com/KhronosGroup/libclcxx). Both of these standard library implementation headers define FP_ILOGBNAN to be INT_MAX:

• https://github.com/llvm/llvm-project/blob/master/clang/lib/Headers/opencl-c-base.h#L165
• https://github.com/KhronosGroup/libclcxx/blob/96459f111c3e3a4709f7e09bf5fb73dea81a475a/include/opencl_math_constants#L85

I've no problem with the compatibility argument. but using INT_MAX for both NaN and Inf is in my understanding wrong.

Again, I didn't see this clearly stated in the spec. Can you quote the spec or point us to the relevant section?

tldr; I don't mind changing this to be in sync with clang, just get the patch right.
However, it's not the right way to guarantee IR level compatibility between clang-cl-headers and libclc.
details below

In D83473#2143394, @bbrezillon wrote:

In D83473#2143152, @jvesely wrote:

What is the problem this patch is trying to address?

Well, the primary goal was to have consistent values in clang/lib/Headers/opencl-c-base.h and libclc/generic/include/clc/float/definitions.h to avoid the mess when one links against libclc but includes opencl-c-base.h.
Not to mention that having 2 conflicting definitions in headers that both lives in the same code base and are supposed to represent the same thing is confusing, to say the least.

What is the problem this patch is trying to address?
The specs do not mandate these two values to be different.

thanks!

In D78979#2006901, @yaxunl wrote:

In D78979#2006847, @arsenm wrote:

I'm also wondering if using -nogpulib for the corresponding linker purpose was correct, since in the OpenCL case it's not really an offload target. Maybe this should switch to -nostdlib?

-nogpulib is fine since opencl compiler is in parallel with the device compiler of CUDA/HIP. The library it uses is the device library.

In D78535#2010720, @tstellar wrote:

This is the failing cmdline (from here https://github.com/tstellar/llvm-project/runs/587088911):

/usr/bin/c++ CMakeFiles/prepare_builtins.dir/utils/prepare-builtins.cpp.o -o prepare_builtins -L/home/runner/work/llvm-project/llvm-project/build/lib -Wl,-rpath,/home/runner/work/llvm-project/llvm-project/build/lib -lz -lrt -ldl -ltinfo -lpthread -lm -lxml2 -lLLVMBitWriter -lLLVMAnalysis -lLLVMProfileData -lLLVMObject -lLLVMTextAPI -lLLVMMCParser -lLLVMMC -lLLVMDebugInfoCodeView -lLLVMDebugInfoMSF -lLLVMBitReader -lLLVMCore -lLLVMRemarks -lLLVMBitstreamReader -lLLVMBinaryFormat -lLLVMSupport -lLLVMDemangle

In D78535#1999173, @tstellar wrote:

In D78535#1998136, @jvesely wrote:

was this a problem with using -Wl,--as-needed?

LGTM.

No, I was hitting this when using the default build options for both LLVM and libclc.

was this a problem with using -Wl,--as-needed?

In D78220#1987377, @nikic wrote:

In D78220#1987284, @jvesely wrote:

Unless we use pointers and an extra level of indirection the end sizes for users like ValueLattice end up the same irrespective of where the union is (lower+upper vs. backend vs. user).

I'd expect that this is indeed what we want to do. Given that the float range is just so much larger, we'll want to allocate it out of line.

In D77923#1983787, @yaxunl wrote:

LGTM. The objective of the change is to simplify offline compilation. We want to avoid asking users to add -D if a proper macro can be inferred from triple and cpu. Ideally, users only need to specify triple and cpu with clang driver. In case users want to override the default macros for the triple, they can always override them by -D or -U in command line.

In D78220#1986237, @fhahn wrote:

In D78220#1985031, @jvesely wrote:

In D78220#1984311, @nikic wrote:

I would strongly recommend to implement this as a separate type (FloatRange for example), rather than combining it into ConstantRange. There's a couple reasons for that, the main ones would be a) float ranges have significantly different semantics, in particular the fact that they are inclusive and b) ConstantRange is a size critical structure, because it is used in value lattice elements. As implement, this change is going to skyrocket memory consumption during SCCP.

+1, I think there is a strong case for not increasing the size here. We should rather try to reduce the size of the existing ConstantRange (using the 2 APInts means we store the same bit width twice :()

The point of having one combined range is to allow simplification of logic for users. you don't have to check for the type just do the same operations union/intersect/binop/contains/...
I'm not sure how splitting it to a separate class would address the memory concerns. Tracking fp ranges will require two APFloats irrespective of the structure.
If keeping both APFloat and APint bounds is a problem I can put them in a union since they are used exclusively and an instance never switches between float and int.

Even with a union, you still need extra space for the bits to distinguish between FP/integers (and other FP related flags, like canBeNaN. It should be possible to achieve the same level of convenience to this patch, but separating integer and FP ranges into separate classes. E.g. add ConstantRangeIntStorage & ConstantRangeFloatStorage which use APInt/APFloat respectively. The shared logic can be implemented in a ConstantRangeCommon template, which can be instantiated with either ConstantRangeIntStorage/ConstantRangeFPStorage. ConstantRange would turn into an instantiation of ConstantRangeCommon<ConstantRangeIntStorage>. Of course that would require updating code to make use of the FP version of ConstantRange. That might be a benefit actually, because there probably are various places that assume ConstantRange only contains integers.

When using the FP version of the constant range in ValueLattice, that would allow us to keep the size the same, if no extra bits (like canBeNaN) are added.

In D78224#1984569, @fhahn wrote:

Very interesting! On a high-level, it would probably be good to split those changes into 3 (ValueLattice, LVI & SCCP); not sure how tricky that's going to be given adding automatic support in markConstant & co. If it's too tricky splitting LVI & SCCP would be great.

In D78220#1984311, @nikic wrote:

I would strongly recommend to implement this as a separate type (FloatRange for example), rather than combining it into ConstantRange. There's a couple reasons for that, the main ones would be a) float ranges have significantly different semantics, in particular the fact that they are inclusive and b) ConstantRange is a size critical structure, because it is used in value lattice elements. As implement, this change is going to skyrocket memory consumption during SCCP.

The point of having one combined range is to allow simplification of logic for users. you don't have to check for the type just do the same operations union/intersect/binop/contains/...
I'm not sure how splitting it to a separate class would address the memory concerns. Tracking fp ranges will require two APFloats irrespective of the structure.
If keeping both APFloat and APint bounds is a problem I can put them in a union since they are used exclusively and an instance never switches between float and int.
The difference in semantics shouldn't impact users much. they should use contains/isEmpty/isFull/isSingleElement/getABCrange instead of checking the bounds manually.
The application to valluelattice/lvi/sccp is in D78224, to keep diff simple it hasn't implemented cleanups that are allowed by having one range class, yet.

There were few issues in SCCP tests.

• apint-load -- the result of the division operation of the original operands depends on rounding mode. it's represented as a range rather than a constant.
• apint-ipsccp4 -- we can now deduce the starting range of v1 in ssa_copy intrinsic as [-Inf, 2.999] which then gets merged with more accurate information from the function call. The original behaviour started with unknown which then gets combined with more accurate information. Thus having less information at the beginning gives a better result. changing the comparison condition check to only allow Icmp works around the issue and keeps the old behaviour.
• float-nan-simplification -- NaNs with different payloads/sign are not tracked separately, so one case returns +NaN while the other one returns -NaN. I've adjusted the comment in the test to explain the situation.

In D77923#1981398, @arsenm wrote:

In D77923#1979286, @jvesely wrote:

In D77923#1976497, @jvesely wrote:

OPENCL_VERSION is something that should be really set by the opencl driver rather than the compiler.
coarse-grained SVM can be done without FEATURE_FLAT_ADDRESS_SPACE, so those gpus can get over 1.2, while the compiler can be used in an implementation that doesn't go above 1.2 even for gfx700+

I don't know why this would imply we can't just have a static __OPENCL_VERSION__ for each device in Clang? I don't see anything in the standard that says you are not allowed to have (going with your example) __OPENCL_VERSION__=200 and __OPENCL_C_VERSION=120 at the same time.

It seems like we should be able to just have __OPENCL_VERSION__ defined in Clang, and let the runtime control setting __OPENCL_C_VERSION__ via -cl-std=. If the current patch sets __OPENCL_VERSION__ differently than the runtime does then it seems like we should make it match, but I'm not sure how best to confirm that.

The __OPENCL_VERSION__ macro in the kernel needs to match the version returned by clGetDeviceInfo (both refer to OpenCL version supported by the device),
which in turn must be <= version returned by clGetPlatformInfo.
This part makes it unsuitable for hardcoding in the compiler.

__OPENCL_VERSION__ should indicate device capabilities rather than language features. specs talk about available resources.
For example, CL_DEVICE_IMAGE2D_MAX_HEIGHT is required to be >= 8192 for OpenCL 1.2 while it's required to be >=16384 for OpenCL 2.1.

But why would the driver not report the device values which we do know in the compiler? A discrepancy between these sounds like a driver bug.

In D77923#1976497, @jvesely wrote:

OPENCL_VERSION is something that should be really set by the opencl driver rather than the compiler.
coarse-grained SVM can be done without FEATURE_FLAT_ADDRESS_SPACE, so those gpus can get over 1.2, while the compiler can be used in an implementation that doesn't go above 1.2 even for gfx700+

I don't know why this would imply we can't just have a static __OPENCL_VERSION__ for each device in Clang? I don't see anything in the standard that says you are not allowed to have (going with your example) __OPENCL_VERSION__=200 and __OPENCL_C_VERSION=120 at the same time.

It seems like we should be able to just have __OPENCL_VERSION__ defined in Clang, and let the runtime control setting __OPENCL_C_VERSION__ via -cl-std=. If the current patch sets __OPENCL_VERSION__ differently than the runtime does then it seems like we should make it match, but I'm not sure how best to confirm that.

OPENCL_VERSION is something that should be really set by the opencl driver rather than the compiler.
coarse-grained SVM can be done without FEATURE_FLAT_ADDRESS_SPACE, so those gpus can get over 1.2, while the compiler can be used in an implementation that doesn't go above 1.2 even for gfx700+

I've checked that custom targets are not cleaned up either, so I think it's good to go.
lgtm

In D77164#1965305, @daniels wrote:

In D77164#1958232, @jvesely wrote:

Have you tried clearing the target link flags using set_target_properties( ... LINK_FLAGS ... )?
I think having the ability to pass LINK_FLAGS here might be useful in the future.

I tried, but the manual says that a) you must use STATIC_LIBRARY_FLAGS for static libraries, and b) you must use STATIC_LIBRARY_OPTIONS in preference to the former, as it sets rather than appends.

Nevertheless, neither of them actually worked for me, and both continued to pass the machine linker flags.

LGTM. thanks

In D77165#1959901, @daniels wrote:

In D77165#1959786, @daniels wrote:

In D77165#1959565, @jvesely wrote:

What was the generator target you used?

I'm using Ninja [...]

As noted in D17762, we have to use Ninja rather than the VS backend, as the 'standard' way to add new languages for compile in CMake doesn't work with the VS backend. CMake's VS backend has a hardcoded-in-C++-code list of acceptable source languages; when you try to produce a static library with sources in an unknown-to-CMake-VS-backend language, the target ends up completely empty with no actions to produce the output, because the CMake backend doesn't know how.

Windows does have pipes. they work both in cmd and powershell, but perhaps cmake is not using shell to invoke these commands?
What was the generator target you used?

In D77162#1957318, @daniels wrote:

I don't see what the second part is trying to address. Does visual studio not detect the static libraries on the command line? I presume the dynamic versions are not present.
If anything it should check llvm-config --shared-mode if available instead of checking for win32. Do all visual studio/clang/mingw use the same flag?

Yes, clang runs as clang-cl which accepts MSVC-type arguments, though you can also invoke clang to pass UNIX-style arguments. For the linker, however, there is no option; lld-link only accepts MSVC-type arguments on Windows and nothing else. I don't know what MinGW does. Is there an easy way to test that? Do you currently build on MinGW?

I could test llvm-config --shared-mode == static && WIN32 to apply /MT, as I tried various other methods of convincing Windows to link statically and not dynamically with no luck. On UNIX this isn't an issue as you can happily mix the two modes.

This will add noise to CMakeOutput.log and CMakeError.log, but I guess it's just one line.

Have you tried clearing the target link flags using set_target_properties( ... LINK_FLAGS ... )?
I think having the ability to pass LINK_FLAGS here might be useful in the future.

I think the first part should use separate_arguments the same way as LLVM_CXXFLAGS does. (you can also add the following change while at it:

-set( LLVM_CXX_FLAGS ${LLVM_CXX_FLAGS} -fno-rtti -fno-exceptions )
+list( APPEND LLVM_CXX_FLAGS -fno-rtti -fno-exceptions )

In D74662#1877586, @tstellar wrote:

In D74662#1877571, @jvesely wrote:

@tstellar , is there a way to make libclc ML an automatic subscriber to phabricator posted libclc patches?

I think you need to ask the phabricator mantainers: https://llvm.org/docs/Phabricator.html#status

ping

@tstellar , is there a way to make libclc ML an automatic subscriber to phabricator posted libclc patches?

Is there any test left that tests the r600 intrinsics? If yes, LGTM.