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

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield 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");
----------------
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.


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:285
+/// host to device memory offloading.
+struct HideMemTransferLatency {
+
----------------
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.


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

https://reviews.llvm.org/D82719