[PATCH] D130466: [LICM] - Add option to allow data races

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

In D130466#3766569 <https://reviews.llvm.org/D130466#3766569>, @gsocshubham wrote:

> In D130466#3761744 <https://reviews.llvm.org/D130466#3761744>, @efriedma wrote:
>
>>> Prohibit this transformation if store points to constant memory using bool PointToConstantMemory.
>>
>> You can't use pointsToConstantMemory to prove the property you need.  The problem is that it doesn't do what you want if it can't prove anything.  You need a function that says the memory isn't writable unless it can prove the memory is writable (maybe call it something like "pointsToWritableMemory").
>
> Understood. Should I add isNotCapturedBeforeOrInLoop() as an extra check as mentioned by @nikic ?

First off, please rebase your patch on main; @nikic made changes in rG315aef66 <https://reviews.llvm.org/rG315aef667ec60675627ecd0fe1424f9c186766de>.

Anyway, with that patch, the relevant bit of code currently looks like this:

  Value *Object = getUnderlyingObject(SomePtr);
  SafeToInsertStore =
      (isNoAliasCall(Object) || isa<AllocaInst>(Object) ||
       (isa<Argument>(Object) && cast<Argument>(Object)->hasByValAttr())) &&
      isNotCapturedBeforeOrInLoop(Object, CurLoop, DT);

There are basically two checks here: one, that the location is readable and writable (it's a malloc/alloca/byval argument/etc,), and two, that the location isn't accessed by some other thread (isNotCapturedBeforeOrInLoop).

If you know there aren't any other threads, you can just skip the isNotCapturedBeforeOrInLoop() check, and you're basically done.

On top of that, you probably also want to expand the list of locations that are known to be readable and writable to include global variables.  (The current code doesn't check for them because the isNotCapturedBeforeOrInLoop() check can't succeed for global variables.)


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

https://reviews.llvm.org/D130466