[PATCH] D98605: [LibCalls] Add `nosync` to known library calls

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/BuildLibCalls.cpp:528
+    Changed |= setNoSync(F);
     return Changed;
   case LibFunc_read:
----------------
jdoerfert wrote:
> aqjune wrote:
> > C17's 7.22.3 Memory management functions has this paragraph:
> > 
> > 2. For purposes of determining the existence of a data race, memory allocation functions behave as though they accessed only memory locations accessible through their arguments and not other static duration storage. These functions may, however, visibly modify the storage that they allocate or deallocate. Calls to these functions that allocate or deallocate a particular region of memory shall occur in a single total order, and each such deallocation call shall synchronize with the next allocation (if any) in this order.
> > 
> > Should we conservatively assume that allocation/deallocation fns may synchronize with other threads?
> My original purpose was to add `nosync` to malloc and free :(
> 
> I guess what this says is that if you have two threads.
> T1: allocate memory, get address P, deallocate it.
> T2: allocate memory, get address P
> 
> You now know T1 deallocated P.
> 
> Worst case we could derive this for call sites if the pointer P was never observed (=captured).
> Worst case we could derive this for call sites if the pointer P was never observed (=captured).

I think it is safer and okay maybe, but since a few malloc implementation such as glibc's one typically has a lock to correctly manipulate allocated areas inside, showing the validity of attaching nosync seems non-trivial to me.

I have a question about the background btw - Does GPU's malloc need to use atomic operations? If GPU's malloc is simpler and they are not guaranteed to synchronize, attaching nosync can be justified.


================
Comment at: llvm/lib/Transforms/Utils/BuildLibCalls.cpp:1100
+    Changed |= setNoSync(F);
     return Changed;
   case LibFunc_abs:
----------------
jdoerfert wrote:
> aqjune wrote:
> > These functions may raise FE exceptions; would it be safe to assume that calling two ldexp, both of which setting FE exceptions, is UB?
> I don't understand the question.
My question was whether math library fns can use atomic operations to update `errno`.

I just found that `errno` is defined as a thread-local storage; I believe it's okay now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98605/new/

https://reviews.llvm.org/D98605