I don't feel it is different for OpenCL though... I am not in favour of repeating the same functionality for every language since the requirement will be likely identical. There is no timeline for when this functionality will be dropped so we have to assume that even though temporary it might be a solution for long enough.

In D108367#2966746, @pekka.jaaskelainen wrote:

@Anastasia I used to have svn commit access, but haven't committed anything for a long while in LLVM, so don't know if I can get them back to github by just asking. So, can you please commit these HIPSPV patches for us?

@Anastasia I used to have svn commit access, but haven't committed anything for a long while in LLVM, so don't know if I can get them back to github by just asking. So, can you please commit these HIPSPV patches for us?

Ok, thanks for clarification. Does it mean there is something we need to add to LLVM somewhere to make it work correctly? Would it be specific to Arm or generally for all CPU targets?

The reason why we would like to fix it is that upstream clang has a crash currently when OpenCL sources are compiled for any Arm CPU: https://bugs.llvm.org/show_bug.cgi?id=50841. Do you have any other suggestions to avoid this problem?

FYI clang also emits kernel metadata that can be used to detect kernels...

Does this break clSetKernelArg() for ARM CPUs in PoCL? I believe so far it has worked for ARM - just wondering why you decide to drop it now?

I glimpsed over this without spotting anything crucial. My Clang code base knowledge is a bit lightweight though so you might want to wait for an another reviewer. On the other hand, the semantics seem to be retained so it might be safe to commit this in case the tests still pass.

Thanks for making this MD more robust! It is essential for the vectorization performance of pocl. Sorry for my slow response time.

Committed in r304389.

Added the requested AMDGCN test case.

In D33639#767046, @yaxunl wrote:

It seems the reviewer is blocked by accident.

LGTM.

In D28080#653493, @Anastasia wrote:

Also, for me the command without -emit-llvm doesn't output anything.

What targets do you use?

In D28080#651732, @Anastasia wrote:

@pekka.jaaskelainen, I just compiled the release 4.0 branch and it all works for me as expected:
clang -cc1 -triple spir64-unknown-unknown test.cl
clang -target spir64-unknown-unknown test.cl

Great work.

In D28048#629569, @Anastasia wrote:

@pekka.jaaskelainen, I think you are using x86 target in the POCL toolchain, do you envision any issues with this change?

Committed in r286819.

target architecture). So if there are no memory segments, nothing can be done to optimize for this and therefore providing
the "fake" segment information doesn't seem to be useful? I am just trying to understand the use case.

Indeed, it requires wider scale discussion to get it right, and e.g. to pass the info to AA. But to be honest, I think OpenCL and CUDA are still considered 'minority' languages in Clang/LLVM which makes me usually lean towards least intrusive implementation solutions whenever possible.

In D26157#585688, @bader wrote:

@pekka.jaaskelainen, I have related question: what is the problem with retaining OpenCL address space information in LLVM IR?
My understanding is that target CodeGen can ignore this information for the machines with 'flat' address space model.
On the other hand I would expect this information be useful for the Optimizer to resolve pointer aliasing.
Does it make any sense to keep OpenCL address space information in LLVM IR generated for the machines with 'flat' address space (e.g. CPU)?

Thanks. @tstellarAMD OK to commit for 3.9.1 as well?

Alright. LGTM!

Makes sense to me. How to differentiate between OpenCL implementations? With the OS setting?

LGTM, FWIW.

In D16876#359781, @Anastasia wrote:

@pekka, do you have any more comments?

LGTM.

LGTM!

Can you add a test that tries to use half precision constants in non OpenCL C mode? Otherwise LGTM.

LGTM.

Otherwise LGTM.

LGTM

@pekka, do you think it would make sense to include a description of the new OpenCL features in the release note too? Or shall we wait for the completion of OpenCL 2.0 support (hopefully in next release)? Could do both as well I guess. :)

LGTM too. Good job. You can try and ask the release manager if you can sneak it in. It's rather container patch so there might be a chance, but I don't know the policy at this stage of the release.

OK. Seems the LLVM 3.8 branching was done, it might be hard to get this in, but should we try and ask the release manager?

In D15914#322917, @pxli168 wrote:

By the way, I just got the access to the llvm svn, can I just commit the pipe type patch directly as I see you all accepted it. Or I should send it to the cfe-commit first?

I wonder could we squeeze this in before the next week's LLVM 3.8 branching? It would be great.

Nice! I will keep on trying to help by reviewing patches whenever time allows.

Is this still waiting for some updates?

Only small issues found, plus it could use some more test cases.

OK now.

Could only find style nitpicks in this one.

We are interested in the pipe.

The patch seems straightforward enough. BTW does someone know if anyone has worked on the 'pipe' qualifier?

The fix looks good, but maybe the test should be an .ir test as this probably affects other targets than MIPS too? Also could be a candidate for 3.7.1?

Is there a matching HLC-HSAIL branch for the latest version of the patch? It doesn't work with with hsail-stable-3.7 anymore.

Perhaps the assert() is unneccessary, but otherwise LGTM.

Can you please clarify me a bit where the actual device is needed/used (ATM only Kaveri) if the target is HSAIL, an "abstract machine"/IL? Just trying to get my head around on how the Clang/LLVM will work for HSAIL.

Great to see this being worked on! It will help the implicit pocl work-group vectorization after the 2-level loop restricting is lifted. Related to this, be careful with the loop id metadata and especially the parallel loop metadata. The parallel loop metadata can be exploited to skip dependency analysis in some cases (At least when both of the loops are parallel? Needs to be thought through.). But if the metadata is moved to a wrong loop during the interchange it might result in a miscompilation.

LGTM.

Are those if conditions that span to the next line indented correctly?

Is this missing the pass itself?

Great, this feature will be really useful. How about naming these metadatas using the llvm.loop. as a prefix? That is, llvm.loop.unroll.enable etc? Maybe it's worth adding a test case with a negative unroll count to ensure nothing bad happens. Otherwise LGTM.