[Openmp-commits] [PATCH] D44487: [OpenMP][libomptarget] Enable globalization for workers

[George Rokos via Phabricator via Openmp-commits]

grokos added inline comments.


================
Comment at: libomptarget/deviceRTLs/nvptx/src/data_sharing.cu:429
+            IsMasterThread() ? DS_Slot_Size : DS_Worker_Warp_Slot_Size;
+        NewSize = DefaultSlotSize > NewSize ? DefaultSlotSize : NewSize;
         NewSlot = (__kmpc_data_sharing_slot *)malloc(
----------------
Can you just use and `if` here? Since `NewSize` does not change value if `DefaultSlotSize <= NewSize`, it's clearer to say
```
if (DefaultSlotSize > NewSize)
  NewSize = DefaultSlotSize;
```



================
Comment at: libomptarget/deviceRTLs/nvptx/src/data_sharing.cu:503-509
+          // Extra
+          Tail->PrevSlotStackPtr = 0;
           Tail = Tail->Prev;
+          // Extra
+          Tail->Next->Prev = 0;
           free(Tail->Next);
           Tail->Next=0;
----------------
I think you can speed things up here. Since you plan to remove the next slot (`free(Tail->Next)`), there is no point in setting new values for the fields of that slot which will be removed right away. You only need to do this for the very final slot (the one which is statically allocated). The two "//Extra" assignments look redundant in this sense. Also, the loop condition can be simplified. Since there is the statically allocated first slot, `Tail` will always point to some slot, it can never be `NULL`, and all you need to check for is `while(Tail->Prev)`. Finally, `Tail->Next = 0` can also be deferred until the end of the loop because the slot `Tail` points to may also be removed in the next iteration. So I think an equivalent (in terms of functionality) version would be:
```
while(Tail->Prev) {
  Tail = Tail->Prev;
  free(Tail->Next);
}
Tail->Next=0;
```



Repository:
  rOMP OpenMP

https://reviews.llvm.org/D44487