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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1054
+        Builder.CreateStore(StoredAggValue, CloneGEP);
+      }
       CI->removeFromParent();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129285