[PATCH] D129285: [OMPIRBuilder] Fill the Aggregate Struct in the serialized parallel region

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1054
+        Builder.CreateStore(StoredAggValue, CloneGEP);
+      }
       CI->removeFromParent();
----------------
kiranchandramohan wrote:
> jdoerfert wrote:
> > This is super fragile. What if there is no GEP for 0 offset or a GEP is reused. If we want something like this we should verify the conditions more closely. E.g., GEP->num_uses() == 1, all users of the CapturedStruct should be GEPs or other things we expect, etc.
> > 
> > Honestly, I would suggest to unify the handling rather than adding special cases. So, always outline, always do the same thing. In the serial case just omit the fork_call indirection.
> > 
> > Would that work?
> I wasn't able to think of a way to unify the handling. The code to outline exists only in one place. So the extractor can be called only once unless we duplicate the parallel region code or modify the extractor to provide two copies or something like that. Were you suggesting something else?
> 
> > This is super fragile. What if there is no GEP for 0 offset or a GEP is reused. If we want something like this we should verify the conditions more closely. E.g., GEP->num_uses() == 1, all users of the CapturedStruct should be GEPs or other things we expect, etc.
> 
> Yeah, I agree that it is fragile and we should add the asserts. And there is always the case that something changes in the extractor's handling of aggregates and then all these will not work.
I'm spitballing here: Could we not call the outlined function in the sequential case too, just without the fork_call indirection? If so, would that solve the issue?
TBH, I'm not sure what the issue is w/o code to look at.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129285