[PATCH] D147172: [OpenMP][Flang][MLIR] Add lowering of TargetOp for host codegen

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4118
+                       OpenMPIRBuilder::TargetBodyGenCallbackTy &CBFunc) {
+  llvm::SmallVector<llvm::Type *, 4> ParameterTypes;
+  for (auto &Arg : Inputs) {
----------------
jsjodin wrote:
> kiranchandramohan wrote:
> > Nit: Is 4 significant here?
> > Nit: Is 4 significant here?
> 
> No, I picked something small that hopefully doesn't require a resize. Not sure if there is a better value to pick?
Doesn't the programmers manual recommend omitting if the value is not significant?
```
In the absence of a well-motivated choice for the number of inlined elements N, it is recommended to use SmallVector<T> (that is, omitting the N). This will choose a default number of inlined elements reasonable for allocation on the stack (for example, trying to keep sizeof(SmallVector<T>) around 64 bytes).```

https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4120
+  for (auto &Arg : Inputs) {
+    ParameterTypes.push_back(Arg->getType());
+  }
----------------
jsjodin wrote:
> kiranchandramohan wrote:
> > In the past we have seen that if the number of live-ins/parameters go above a certain number that leads to ABI issues on some platforms.
> > The OpenMPIRBuilder flow packs the arguments into a struct and then unpacks it in the outlined function. Is this not a concern for offloading?
> > In the past we have seen that if the number of live-ins/parameters go above a certain number that leads to ABI issues on some platforms.
> > The OpenMPIRBuilder flow packs the arguments into a struct and then unpacks it in the outlined function. Is this not a concern for offloading?
> 
> Clang does not pack arguments into a struct for the outlined fallback (host) for target regions. It could be that the number of parameters are fewer compared to other outlining cases. 
> It might be desirable to use the kernel args structure so both the host and device use the same argument passing, but this is not done today.
OK. It is fine for now.


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

https://reviews.llvm.org/D147172