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

[Shubham Narlawar via Phabricator via llvm-commits]

gsocshubham added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2119
   if (!SafeToInsertStore) {
-    if (IsKnownThreadLocalObject)
+    if (IsKnownThreadLocalObject || AllowDataRaces)
       SafeToInsertStore = true;
----------------
hiraditya wrote:
> xbolva00 wrote:
> > xbolva00 wrote:
> > > gsocshubham wrote:
> > > > gsocshubham wrote:
> > > > > xbolva00 wrote:
> > > > > > fhahn wrote:
> > > > > > > gsocshubham wrote:
> > > > > > > > efriedma wrote:
> > > > > > > > > I think you need a few more safety checks, even if you're assuming the program is single-threaded.  Just because it's legal to load from a memory location, doesn't mean it's legal to store to it.  For example, you can't write to a constant.
> > > > > > > > I am not sure about writing to a constant. Can you elaborate with an example where it would fail?
> > > > > > > This doesn't like quite right. You are just comparing the enum value, not the actual configured ThreadModel I think. You likely have to check the value returned by `getThreadModel`.
> > > > > > and no tests?
> > > > > `getThreadModel()` is declared here - https://github.com/llvm/llvm-project/blob/d0541b47000739c68c540170c6b9790ec1ea3b77/llvm/include/llvm/CodeGen/CommandFlags.h#L42
> > > > > 
> > > > > Defination is present in clang - https://github.com/llvm/llvm-project/blob/448adfee05b737a26dda34e7ae2cd4948760fff0/clang/include/clang/Driver/ToolChain.h#L573
> > > > > 
> > > > > ` virtual std::string getThreadModel() const { return "posix"; }`
> > > > > 
> > > > > I am not sure on how can I use `getThreadModel()` in LICM.cpp. Is there any other way to know ThreadModel?
> > > > > 
> > > > I have added a test here - `hoist-load-without-store.ll` where load is hoisted out of loop.
> > > > 
> > > > I did not get what do you mean by no tests?
> > > So maybe clang can run llvm with
> > > 
> > > -mllvm -your-new-flag-to-allow-data-races 
> > > 
> > > if threadmodel is single?
> > Tests that we perform this optimization only when threadmodel is single
> Yeah, we need test checks the optimization was performed when threadmodel is single, and does not do the optimization otherwise.
1. Passing `-mllvm -your-new-flag-to-allow-data-races` to clang will work with my original submission - in which there is a new flag AllowDataRaces in LICM.

2. I have tried an approach to let LICM know whether thread model is single or not as below -

a. In `clang/lib/CodeGen/BackendUtil.cpp`, we have thread model info using `LangOpts.getThreadModel()`

b. I pass a bool variable to PassBuilder which in turns pass to LICM.

c. LICM based on this bool variable knows whether to hoist the load from the loop if thread model is single or not. I did a trail approach by referring to `AllowSpeculation` variable which is being passes to LICM. I don't think it is optimal/suggested solution.

>From my other comment, first I want to get clear with `-mthread-model single` option. Can you you comments on it?


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

https://reviews.llvm.org/D130466