[SYCL] Add Windows support for device_info #147
Conversation
sycl/source/detail/os_util.cpp
Outdated
| ModuleInfo *MI = (ModuleInfo *)data; | ||
| static int callback(struct dl_phdr_info *Info, size_t Size, void *Data) { | ||
| unsigned char *Base = reinterpret_cast<unsigned char *>(Info->dlpi_addr); | ||
| ModuleInfo *MI = (ModuleInfo *)Data; |
There was a problem hiding this comment.
Please, no old school C cast...
There was a problem hiding this comment.
Fixed. Thank you.
sycl/source/detail/os_util.cpp
Outdated
| unsigned char *Base = reinterpret_cast<unsigned char *>(Info->dlpi_addr); | ||
| ModuleInfo *MI = (ModuleInfo *)Data; | ||
| const unsigned char *TestAddr = | ||
| reinterpret_cast<const unsigned char *>(MI->VirtAddr); |
There was a problem hiding this comment.
In all these 3 lines, the type is already expressed in the reinterpret_cast, so use auto Base = and so on.
And then it should fit on 1 line and be even clearer.
There was a problem hiding this comment.
Ok, fixed. Thank you.
sycl/source/detail/os_util.cpp
Outdated
|
|
||
| // check if the tested address is within current segment | ||
| if (TestAddr >= SegStart && TestAddr < SegEnd) { | ||
| // ... it is - belongs to the module then | ||
| // dlpi_addr is zero for the executable, replace it | ||
| void *H = (void *)info->dlpi_addr; | ||
| void *H = (void *)Info->dlpi_addr; |
There was a problem hiding this comment.
I think you can remove (void *)
There was a problem hiding this comment.
(void ) cannot be just removed here. Otherwise, the error is emitted due to conversion of 'long unsigned int' to 'void'.
I replaced it with reinterpret_cast.
There was a problem hiding this comment.
Ah, right. I though info->dlpi_addr was an address already, so another kind of pointer.
Sorry about the noise...
|
|
||
| size_t OSUtil::getOSMemSize() { | ||
| #if defined(SYCL_RT_OS_LINUX) | ||
| struct sysinfo MemInfo; |
There was a problem hiding this comment.
In C++ probably
sysinfo MemInfo;
is enough.
There was a problem hiding this comment.
In this case 'struct' keyword is needed. Otherwise the error is emitted.
There was a problem hiding this comment.
Ah, very curious.
Perhaps because in int sysinfo(struct sysinfo *info); sysinfo is the name of a function and the name of a structure at the same time, so it is ambiguous?
| @@ -16,7 +16,6 @@ namespace cl { | |||
| namespace sycl { | |||
| template <int dimensions> class range; | |||
| template <int dimensions = 1> struct id : public detail::array<dimensions> { | |||
| public: | |||
There was a problem hiding this comment.
I agree that replacing class by struct is a simplification in the current situation.
But I wonder whether the original author did not mean instead having the next line with base private, so that public: was indeed on the wrong line...
Is there a reason to have base publicly visible?
There was a problem hiding this comment.
Well, the main goal of all those changes ("class" --> "struct") was the sync with SYCL SPEC and the wish to avoid build warnings like "this class was initially declared as struct".
In the additional change today I fixed 'id.hpp' and 'range.hpp' to hide 'base' inside private section.
There was a problem hiding this comment.
Hi Ronan,
Please approve this PR if you agree with the additional changes.
Thank you,
Vyacheslav
Fixed few warnings caused by inconsistent usage of class and struct keywords. Added a macro to expose global symbols and generate sycl.lib on Windows. Signed-off-by: Vyacheslav N Klochkov <[email protected]>
CONFLICT (content): Merge conflict in clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp CONFLICT (content): Merge conflict in clang/test/Driver/clang-offload-bundler.c
Since we (Linaro) moved out bots to a new machine, this test has been failing: https://lab.llvm.org/buildbot/#/builders/121/builds/1566 Most of the time, the rss difference is greater than 512 on the first iteration then settles down to 512 for all the rest. ``` starting rss 512 shadow pages: 1024 p = 0xe083e0800000 1536 -> 740 diff 796 1252 -> 740 diff 512 1252 -> 740 diff 512 1252 -> 740 diff 512 1252 -> 740 diff 512 1252 -> 740 diff 512 1252 -> 740 diff 512 1252 -> 740 diff 512 1252 -> 740 diff 512 1252 -> 740 diff 512 p = 0xe083e0800000 passed 1 out of 10 release-shadow.c.tmp: /home/tcwg-buildbot/worker/clang-aarch64-lld-2stage/llvm/compiler-rt/test/hwasan/TestCases/Linux/release-shadow.c:81: int main(): Assertion `success_count > total_count * 0.8' failed. ``` Given that the test was looking for a diff of at least 513, I guess that 512 is ok too. For future reference, the original bot host was running this kernel: Linux 5.15.0-136-generic #147-Ubuntu SMP Sat Mar 15 15:51:36 UTC 2025 aarch64 aarch64 aarch64 GNU/Linux And the new host: Linux 6.8.0-64-generic #67-Ubuntu SMP PREEMPT_DYNAMIC Sun Jun 15 20:23:40 UTC 2025 aarch64 aarch64 aarch64 GNU/Linux Though the new host also has more RAM, so the kernel may be less aggresive with memory management.
Fixed few warnings caused by inconsistent usage of class and struct keywords.
Added a macro to expose global symbols and generate sycl.lib on Windows.
Signed-off-by: Vyacheslav N Klochkov [email protected]