[PATCH] D85544: [OpenMPOpt] ICV tracking for calls

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Looks much better now. Mostly minor comments.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:945
     }
   }
 };
----------------
Nit: We can now get rid of the outer loop and the conditional as well by using the SCC version of foreachUse. 


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1240
+          HasChanged = ChangeStatus::CHANGED;
+        }
+        return true;
----------------
Don't we need to check if we already assumed this value?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1252
+      if (HasChanged == ChangeStatus::CHANGED && !ValuesMap.count(Entry))
+        ValuesMap.insert(std::make_pair(Entry, nullptr));
     }
----------------
What is this about?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1256
     return HasChanged;
   }
 
----------------
I feel this update doesn't need to look at everything but only the things we have in the cache. Right now it will look at all calls for all ICVs, which is probably not necessary.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1273
+    if (CalledFunction == GetterRFI.Declaration)
+      return None;
+    if (CalledFunction == SetterRFI.Declaration) {
----------------
Arguably, the value for the ICV is the getter call, so I.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1458
+      }
+    }
+
----------------
This seems unfortunate but OK for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85544