RWTH Aachen University
- User Since
- Apr 2 2015, 4:52 AM (146 w, 2 d)
Thu, Jan 18
Use llvm::Twine, add PackedAttr, and adjust regression tests to check the name of the generated constants.
Wed, Jan 17
Honestly I don't really care, but please keep the indention in ompt_try_start_tool or move the comment so that clang-format doesn't get it wrong.
Hmm, this should generally be done for each change and not for the sake of formatting: https://llvm.org/docs/CodingStandards.html#introduction
Thu, Jan 11
Wed, Jan 10
I'm not sure why you need to treat KMP_COMPILER_GCC differently, it's the test compiler that is relevant, isn't it?
Hmm, can we also somehow test that the runtime continues searching for a tool in that case? For example having OMP_TOOL_LIBRARIES with a tool that has no ompt_start_tool, one that returns NULL and a final one that does activate OMPT?
Test looks good to me.
Tue, Jan 9
Mon, Jan 8
I think I saw a few failures related to return addresses on my Raspberry Pi 2... I'll try to retest asap.
Sun, Jan 7
Two global remarks:
- I think we agreed on having <thread id> % <warp size> instead of bit operations.
- Somewhat related, there are currently at least four different "symbols" for the warp size: warpSize, WARPSIZE, DS_Max_Worker_Warp_Size, and DS_Max_Warp_Number. This needs to be exactly one that is used throughout the runtime.
Thu, Jan 4
Does the bitcode lib work with upstream Clang trunk?
Tue, Jan 2
LG. I'd feel better with an explicit acknowledgment from Intel though...
Mon, Jan 1
Use memset instead of custom touch function.
Sun, Dec 31
Thu, Dec 28
Not needed anymore, we decided on the order of arguments in D41012.
Wed, Dec 27
The added test has been disabled on armhf in rCRT320665 because it fails a bot: http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2952/steps/ninja%20check%202/logs/FAIL%3A%20Scudo-armhf%3A%3A%20interface.cpp
Looks good so far. Can you document the change of the default value in the (just committed) README.rst?
Sat, Dec 23
Does the latest change assume current_task == new_taskdata->td_parent (which would make sense to me)? If not this change has more implications than the frame address.
Fri, Dec 22
I don't think we should enable this unconditionally, only for the architectures where we tested and are sure that the tests will pass: x86, x86_64, ppc64, aarch64 on Linux, macOS if we want to. With the current change we will get into trouble at least on MIPS(64).
Dec 21 2017
Address review comments and pay attention to protected names.
D41012? This patch doesn't update the documentation with function signatures.
Ping. Is this ok to land before the 6.0 branch?
Removed documentation about the NVPTX runtime for now which isn't committed yet.
@hfinkel I think you requested this documentation on the mailing list. Can you take a look if it matches your expectations so we can get this bundled in the 6.0 release?
Dec 20 2017
LG. This fixes the test with GCC which already had the enter address, right?
Makes sense in some way. However if I read the spec correctly the tool gets an unsigned int. So maybe we should keep the enum in the header but prefix it with kmp instead of ompt to make clear that this is not portable? This would allow tools to conditionally compare the passed value
LG but please describe which test cases are fixed by this change (and possibly why it is needed). If no test case has been failing before, this is a sign that we need a new one!
Dec 13 2017
Can we have a test for this? Like setting the callback, unsetting it again and checking that the function isn't called?
This needs a test.
Two small things to considers, looks good otherwise.
LGTM if you add a comment in the code explaining why we need this (old format) and that this effectively disables error detection in the new format on AArch64