[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;
----------------
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?



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2119
   if (!SafeToInsertStore) {
-    if (IsKnownThreadLocalObject)
+    if (IsKnownThreadLocalObject || AllowDataRaces)
       SafeToInsertStore = true;
----------------
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?


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

https://reviews.llvm.org/D130466