[PATCH] D82719: [OpenMPOpt][SplitMemTransfer][WIP] Getting values stored in offload arrays

[Hamilton Tobon-Mosquera via Phabricator via llvm-commits]

hamax97 added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:255
+  /// Returns the integer representation of \p V.
+  static int64_t getIntLiteral(const Value *V) {
+    assert(V && "Getting Integer value of nullptr");
----------------
JonChesterfield wrote:
> This looks wrong. If V is always a ConstantInt, we want cast. If it isn't, then dyn_cast will return null and this will crash on the dereference.
You're right. I'll fix it. It seems weird though, what should I return then?. Probably not having this function is a better idea


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:285
+/// host to device memory offloading.
+struct HideMemTransferLatency {
+
----------------
JonChesterfield wrote:
> hamax97 wrote:
> > jdoerfert wrote:
> > > sstefan1 wrote:
> > > > hamax97 wrote:
> > > > > IMHO it would nice to have the optimizations outside OpenMPOpt. It's easier testing them like this, and a good practice according to Googletest, [[ https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#testing-private-code | Testing private code ]].
> > > > > It's also easier adding new features to them.
> > > > I'm ok with having this here. I guess it would be good to add some documentation comments in the .h files.
> > > I'm not convinced that we need to expose an optimization that runs as part of OpenMPOpt (only) to the world.
> > > 
> > > Why do we need all this code in the (public) header? If you really want to separate it, I guess a private header in the lib folder would suffice, right? Or do you expect other code to interact with this?
> > Well, the idea is to make this easily testable. So, if having a private header, we will need to include it in the unittests. If that is not a problem, then it is a better idea having the private header. What do you think then?
> If stuff needs to be a header in order to poke at it from unit tests then so be it.
> 
> OpenMPOpt.h is the header developers will look at to see how to invoke the optimisation, so it's better to keep implementation cruft out of it.
> 
> So a second header is probably the right compromise.
Do you mean a private header inside `lib/` or a public one inside `include/`?.


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

https://reviews.llvm.org/D82719