[SYCL] Fixing device check in program link constructor #74
Conversation
schittir
assigned bader, romanovvlad, kbobrovs, vladimirlaz and v-klochkov and unassigned bader, romanovvlad, kbobrovs, vladimirlaz and v-klochkov
| std::map<cl_device_id, device> IdToDeviceMap; | ||
| for (auto &Dev : SyclContextDevices) { | ||
| IdToDeviceMap[Dev.get()] = Dev; | ||
| } |
There was a problem hiding this comment.
It is not clear to me whether when you are calling sort_devices_by_cl_device_id() you have gone always through here. If so, since you are using std::map<cl_device_id, device>, if you are iterated on it it is already sorted by cl_device_id
There was a problem hiding this comment.
The map is used only in the interoperability constructor, whereas the sorting_by_cl_device_id() for device check is called in the program link constructor. The programs being linked may or may not constructed through the interoperability constructor.
I don't know if std::sort is optimized for already sorted containers but I can add an if(!is_sorted(sortedDevices.begin(),sortedDevices.end()) check before calling the sort method. I wonder if it is it really important to do so.
There was a problem hiding this comment.
No I do not think this micro-optimization is important.
But thank you for the clarification.
And since you have it at hand, feel free to add it as a comment somewhere.
Comments are super useful for reviewers and generally for any non write-only code. ;-)
Thanks.
| // Returns underlying native device object (if any) w/o reference count | ||
| // modification. Caller must ensure the returned object lives on stack only. | ||
| // It can also be safely passed to the underlying native runtime API. | ||
| // Warning. Returned reference will be invalid if device_impl was destroyed. |
There was a problem hiding this comment.
I am a little bit scared by reading these 2 lines at the same time...
There was a problem hiding this comment.
The purpose of the first three lines is to explain the usage of the method and the responsibility of the user. The warning is intended to be scary and is consistent with other classes containing getHandleRef method.
The juxtaposition seems useful to me and is used in other classes too.
This patch gets sycl devices via context in program interoperability constructor and sorts devices in program link constructor to check that all the programs in the list use the same devices It also addresses the case where program is constructed using only a subset of devices associated with sycl context Signed-off-by: Sindhu Chittireddy <[email protected]>
CONFLICT (content): Merge conflict in clang/include/clang/Basic/DiagnosticSemaKinds.td
CONFLICT (content): Merge conflict in clang/lib/Driver/ToolChains/Clang.cpp
CONFLICT (content): Merge conflict in clang/lib/Driver/ToolChains/Linux.cpp
This patch gets sycl devices via context in program
interoperability constructor and
sorts devices in program link constructor to check that
all the programs in the list use the same devices
Signed-off-by: Sindhu Chittireddy [email protected]