RWTH Aachen University
- User Since
- Apr 2 2015, 4:52 AM (220 w, 5 d)
Fri, Jun 21
The fixes look good overall, but I'd like approval from somebody more familiar with this part of the code.
Thu, Jun 20
Even though I know you didn't send the email yet, can you please upload the diff with full context? Thanks :-)
Mon, Jun 17
Sat, Jun 15
Am I correct that the second to last revision ("- Fix tests.") removed all checks for the actual device_id argument from the tests? From my point of view that's not fixing but weakening the tests! Can you explain why they needed "fixing"?
Wed, Jun 12
Trying to catch up on all the comments, replies to some:
Mon, Jun 10
As for D63010, the new function isn't used anywhere (and hence cannot be tested). What's the advantage of splitting changes at such granularity?
Is there a particular reason for this kind of micro-patches? The function is not made available in the linker script and there's no implementation either.
Tue, Jun 4
Oh, LLVM_ENABLE_FFI is off by default? That's not good, because we rely on the plugins to test libomptarget. So changing the default will have a negative impact on test coverage. I'm afraid that's not a good idea, even if we are now using libffi when LLVM_ENABLE_FFI = Off. We could still add LIBOMPTARGET_ENABLE_FFI (at the top-level CMakeLists.txt, please) and default to on, but I'm not sure if that's of much help.
Mon, Jun 3
We support standalone builds of openmp where LLVM_ENABLE_FFI will not be defined. Can you add a check for OPENMP_STANDALONE_BUILD OR or DEFINED LLVM_ENABLE_FFI? Also please try to be consistent with the surrounding style, ie no whitespace after the opening and before the closing parenthesis.
Tue, May 28
May 26 2019
Please subscribe to openmp-commits for future patches to the OpenMP runtime such that your revisions from Phabricator and more importantly commit emails get through.
May 25 2019
Looks mostly good, one last question about the return type of __tgt_rtl_init_requires that I noticed only now.
May 22 2019
Some high-level comments inline
May 21 2019
LG in case we don't need __tgt_register_requires to return a code in the near future (like for saying "that's not supported")
May 18 2019
In this patch __tgt_rtl_init_requires does nothing. Do we really want to add the prototype now? If yes, are we sure that we won't need to pass more information in the future? Once we add it, the runtime library must maintain compatibility.
May 15 2019
May 8 2019
If possible we should also have a test for ompt_finalize_tool.
May 6 2019
May 1 2019
My understanding of enclosed is that its meaning is recursive. So omp_in_parallel is to return true iff (at least) one of the surrounding parallel regions is active. That also matches the summary in OpenMP 5.0 which says
The omp_in_parallel routine returns true if the active-levels-var ICV is greater than zero; otherwise, it returns false.
active-levels-var - the number of nested active parallel regions that enclose the current task such that all of the parallel regions are enclosed by the outermost initial task region on the current device. There is one copy of this ICV per data environment.
Apr 26 2019
Apr 20 2019
So the scheme is: pow is defined in __clang_openmp_math.h to call __kmpc_pow. This lives in libomptarget-nvptx (both bc and static lib) and just calls pow which works because nvcc and Clang in CUDA mode make sure that the call gets routed into libdevice?
Why is it enough to have one counter per warp, what happens if threads within a warp diverge? Before D55773 we had a counter per thread...
Apr 15 2019
Apr 13 2019
Apr 11 2019
Apr 5 2019
Apr 2 2019
AFAICS all comments were addressed and this looks good. Please wait a day or so in case I missed something
I guess that change makes sense overall, some comments from reading the code inline. I didn't apply the patch locally so I might be missing some context here...
Mar 27 2019
Most of the changes look pretty straight forward to me, but I currently don't have time to read through the assembly implementation of __kmp_invoke_microtask. I left two comments inline to discuss compressed vs uncompressed encodings for testing OMPT.
Mar 24 2019
The code in cmake/OpenMPTesting.cmake should also handle standalone builds of openmp.
Mar 17 2019
Mar 15 2019
Okay, the test failure is related to my Intel Westmere system (yeah, that's old, I know) where I see:
b = 0x13c0 kBmi12Mask = 0x108
Even the "corrected" code only checked that at least one bit is set :-(
Mar 13 2019
Mar 12 2019
Hi Doru, I think this patch is not yet in trunk? Additionally I'm seeing the same warnings when compiling with nvcc which doesn't seem addressed here. Should we pass -std=c++11 when compiling the CUDA code as well?
Ping. As I said I think the current approach with a fixed-size buffer makes sense because anything larger likely won't work anyhow.
Mar 11 2019
I think the change is correct, but I don't know enough of the code to feel confident.
Mar 10 2019
The added test is failing for me because I'm building (and testing) compiler-rt with Clang 7 which doesn't instrument BMI, and I'd assume the same with Clang 8 once released. Is that an oversight or is this configuration expected to break?
Mar 6 2019
Mar 5 2019
Mar 4 2019
Mar 3 2019
Move common ELF description to Inputs/.
Mar 2 2019
Replace llvm_unreachables in Object/ELF.cpp by break statements and add test to exercise this code path.
Mar 1 2019
I think this patch is right in also sorting the function names: AFAICS StringMap doesn't provide that guarantee.
Feb 28 2019
I don't care how this is solved, but it needs to be solved! And it's not getting easier with everyone saying something different