Remove duplicated switch over builtins.

Use CHECK-LABEL.

@tra is there anything else I should do for this patch? Thank you.

append -> +

Simplify the check for half tys support.

Address PR comments

Add more verbose test names.

Looks good.

In D135428#3850180, @tra wrote:

In D135428#3848687, @jchlanda wrote:

In all honesty I don't know what the motivation for this was, it came to my attention as a DPC++ bug (https://github.com/intel/llvm/issues/6958). I do think that your point about flushing behavior is important and should be preserved.

FWIW, using negdirectly does not require a bitcast from Float16x2Regs to Int32Regs or Float16Regs to Int16Regs, as seen in the xor case.

Such bitcasts are essentially no-ops once ptxas is done with them. PTX ends up being a bit more verbose, but it usually has no impact on the SASS. FP and integers are kept in the same registers on the actual hardware. I've commented on the original bug.

Anyways, I think this change is fine. I just wanted to make sure I'm not missing something.

In D135428#3843292, @tra wrote:

Just curious -- what prompts this change?

Does it buy us anything performance-wise? AFAICT llvm may be generating better code for gpus w/o fp16 support -- it does xor on 32-bit value w/o splitting it into 16-bit halfs. https://godbolt.org/z/Wjx7ceT75
Or is it needed to flush fp16 denormals consistently?

Use helper functions when handling address space values.

In D124382#3480600, @Anastasia wrote:

@Anastasia @tra apologies for a late reply, I'm catching up with the thread after holidays.

In D124382#3472888, @Anastasia wrote:

@tra @Naghasan @t4c1 you might find it interesting, a follow up from the discussion here: https://reviews.llvm.org/D112718

@tra would you be so kind and merge it in for me please?

In D117887#3352079, @tra wrote:

In D117887#3351257, @jchlanda wrote:

@tra thank you for landing the patches, it seems that the clang part (builtin declarations and tests) have been dropped, only llvm dir changes made it through. Is there any way I could fix it (same goes for the other two patches in this stack)?

Somehow arc export | git apply didn't pick clang changes when I was transferring the patch from the phabricator. I'll re-fetch the patches and will land the missing pieces shortly.

I'm not sure how the you've submitted the patch to phabricator. In general, it works best when the patch is supplied as a gic commit diff, with the author metadata, etc.
Or via arc diff. See for the details. https://llvm.org/docs/Phabricator.html#phabricator-reviews

@tra thank you for landing the patches, it seems that the clang part (builtin declarations and tests) have been dropped, only llvm dir changes made it through. Is there any way I could fix it (same goes for the other two patches in this stack)?

@tra I've fixed the test failure (math-intrins.ll) the rest seems to be unrelated timeouts, would you be able to merge those patches in, as I don't have the commit access please? The same goes for https://reviews.llvm.org/D117887 and https://reviews.llvm.org/D119157 from Nicolas. Thanks.

PTX/sm version tidy up.

Tidy up FMA_TUPLE class.

In D118977#3302158, @tra wrote:

In D118977#3299974, @jchlanda wrote:

Target ISA Notes
Requires sm_53 or higher.

I think we do need this constraint applied to the new builtins, too. Right now nothing stops using them on a GPU where they do not exist and that will likely crash the compiler when we fail to find a matching intrinsic.

Add sm/ptx version guard to f16{x2} builtins.

In D118977#3297465, @tra wrote:

They all require PTX 7.0, SM_80.

According to https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#half-precision-floating-point-instructions-fma only fma.relu and bf16* variants require ptx70/sm80:

PTX ISA Notes
Introduced in PTX ISA version 4.2.

fma.relu.{f16, f16x2} and fma{.relu}.{bf16, bf16x2} introduced in PTX ISA version 7.0.

Target ISA Notes
Requires sm_53 or higher.

fma.relu.{f16, f16x2} and fma{.relu}.{bf16, bf16x2} require sm_80 or higher.

Set correct SM and PTX version.

In D117887#3262024, @tra wrote:

Added xorsign.abs variant and test.

In D117887#3262024, @tra wrote:

I need to update this to the upstream, as it was based on intel's fork. Abandoning for now.

add_dependencies actually should work fine, regardless of the ordering. The problem is you're checking for the existence of the target.

Stop doing that.

This is actually not safe. You're creating a new cached value based on the value of another cached value. Both can be set and updated independently of each other. You should (at least) add a hard error if SYCL_INCLUDE_TESTS=On and LLVM_INCLUDE_TESTS=Off.

SYCL already predicates building unittests on the value of LLVM_INCLUDE_TESTS (https://github.com/intel/llvm/blob/sycl/sycl/CMakeLists.txt#L212). The problem is that at the point of time when check-sycl is built gtests are not known to CMake (even though SYCL code did check for the existence of the source), so it is impossible to add them to the list of dependencies. Yes they will be built (if the LLVM_INCLUDE_TESTS is set on), but the dependency will not be set up correctly and depending on the order in which the targets are built check-sycl might fail to link. It boils down to inter-target dependencies in CMake.

In D104848#2910218, @beanz wrote:

What are you trying to do in a project that is motivating this?

I don’t have any real objection to this change, but the motivation concerns me. Generally we avoid things that require order-dependence, and setting up target dependencies does not.

ping