- User Since
- Aug 11 2015, 1:08 PM (231 w, 3 d)
Tue, Jan 14
Thanks for the change to uint64_t
- Avoid trip through interface for target_impl
Mon, Jan 13
The plumbing looks sane to me. I imagine any bugs or limitations we miss at review will make themselves apparent as more complicated optimisations are included.
Mostly minor comments. You've got a lot of mileage out of x macros. I really like the unit tests generated from the same .def as the source.
Change looks good to me as is. Some notes / questions on the reference counting.
Fri, Jan 10
Nice catch, thanks
Fri, Jan 3
Great! Thank you
Mon, Dec 30
Ah, right. I follow. Some groundwork to be done first then, thanks.
I"d like to understand that use case. Someone compiles libomp from trunk, pastes it over the version from repos, but leaves the rest of the toolchain unchanged. Breakage seems a fair result.
Do we have a consensus / policy on ABI breaks?
I like this, thanks. Very clear.
Mon, Dec 23
The premise seems OK, but three copies of a large block of control flow is not so good. Why the duplication?
Big patch but looks like a net decrease in complexity. Please could you clang format the areas phabricator is complaining about?
Sat, Dec 21
Fri, Dec 20
Looks good, though I haven't built it to check the tests still pass. I take it this is the NFC subset of the previous patch?
Thu, Dec 19
Dec 18 2019
Dec 17 2019
Copying this file into the trunk source tree builds for me (using parts of a local aomp build)
- Change to template implementation
- Change to template implementation
It seems to be an openmp restriction that parallel regions can't exit/abort/return etc, so the current path looks fine. Perhaps an assertion on the attribute?
I think next steps from here are:
- get the file building against trunk
- work out what the requirements on ATMI are - I think ROCm and AOMP can use the same version, but haven't tested carefully enough to be sure yet
- clean up the fprintf(stderr...) noise
- try to simplify the cmake searching logic
Aside from the inline comment about functions that don't return, this looks good to me. Simple code, heavily tested. Thanks
Dec 16 2019
Apologies for changing the patch post review. The print calls are now back in the omp_ prefixed functions. That seems a better separation of concerns.
- Move print statements back into generic functions
- More explicit cast
Thanks. This is most of the target specific code in libcall, the remaining function is for omp_get_wtime. I'll put up a patch for that shortly.
Ping. I'm blocked until this is resolved.
Dec 14 2019
One would get missing symbols if objects compiled against the previous support.h were linked to ones compiled against the files here, e.g. if one attempted an incremental build of deviceRTL despite the dependency graph changing between revisions.
Dec 13 2019
All passes fine for me, with a debug build using clang 9. Symbols in libraries look fine too.
Thanks. There's something very wrong here - every compilation unit can see the inline definition (else they wouldn't compile), so debug is throwing away the definition without cause to believe there will be an implementation in another TU.
Which build? Debug/release nvcc/clang?
Reverted in dd8a7fcdd73dd63529b81bf9f72c7529dfe99ec3
Breaks NVPTX tests with unresolved symbols.
Nice feedback, thanks!
Nice cleanup, thanks
That sounds pragmatic. A single header located under common/ will work for nvptx and amdgcn without #ifdef, so lets go with that. We can rearrange the code if necessary when a third target arises or when amdgcn/nvptx wants to implement the atomics differently.
Has anyone actually asked Richard to look at this? He isn't subscribed to the diff and may not be watching openmp-dev.
Couldn't we provide these functions in the target_impl.h of amdgcn?
Dec 12 2019
There's an outstanding design point here.
__kmpc_atomic_foo works for me, as does running sed once.
Partially guarding against a user accidentally or incompetently modifying a binary isn't sufficiently useful to justify adding code to lld in my opinion.