[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;
----------------
gsocshubham wrote:
> gsocshubham wrote:
> > 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?
> If we pass thread model information as above, then we don't need flag AllowDataRaces at all. But I think there should be more efficient way apart from above - on how clang driver pass information to llvm passes.
@hiraditya - I have added test `LICM/without-allow-data-race.ll` which shows that optimization is not done when flag is not set - just for confirmation purpose.

I think that it will be redundant test compared to `LICM/promote-sink-store.ll` and I should probably remove it once patch is ready to commit.


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

https://reviews.llvm.org/D130466