jdoerfert marked an inline comment as done. jdoerfert added inline comments.
================ Comment at: openmp/libomptarget/deviceRTLs/common/target_region.h:104 +/// +EXTERN int8_t __kmpc_target_region_kernel_init(ident_t *Ident, bool UseSPMDMode, + bool RequiresOMPRuntime, ---------------- ABataev wrote: > jdoerfert wrote: > > ABataev wrote: > > > jdoerfert wrote: > > > > ABataev wrote: > > > > > If you're using `ident_t` `UseSPMDMode` and `RequiresOMPRuntime` > > > > > parameters are not needed anymore. They are passed in `ident_t` > > > > > structure. > > > > > If you're using ident_t UseSPMDMode and RequiresOMPRuntime parameters > > > > > are not needed anymore. They are passed in ident_t structure. > > > > > > > > They are not in the TRegion interface, at least not by the TRegion code > > > > generation. If required, we can add that or require the > > > > `__kmpc_target_region_kernel_init` implementation to store the values > > > > in the `ident_t`. Regardless, we do not want to hide the variables in > > > > the `ident_t` because that would result in worse analysis results and > > > > cause optimizations to be harder. The main point of all these changes > > > > is, after all, to make optimizations easy. Given that we expect these > > > > functions to be inlined, there is also no harm done wrt. runtime costs. > > > > > > > > > > > > > > > > > > > This is why we used them. Those `ident_t`s are constant and it allows us > > > to perform an additional optimization in the functions, that do not have > > > `isSPMDMpde` and `RequiresFullRuntime`. Because of this parameter, we > > > gained a significant performance boost. LLVM knows how to deal with the > > > structures, don't worry about the optimization. > > > This is why we used them. Those ident_ts are constant and it allows us to > > > perform an additional optimization in the functions, that do not have > > > isSPMDMpde and RequiresFullRuntime. > > > > The boolean parameters are (currently) also constant. The main point > > however is that in our expected use case, an inlined device RTL, there is > > literally no cost to pay by having the flags explicit as parameters. > > > > > > > Because of this parameter, we gained a significant performance boost. > > > > Compared to what? Not having information about the execution mode, etc. at > > all? How would that become worse? > > > > > > > > > > > LLVM knows how to deal with the structures, don't worry about the > > > optimization. > > > > I am (painfully) aware of LLVM's capability to promote arguments (that is > > what is needed if we do not inline or perform IP-SCCP). However, using a > > pointer does allow the use of non-constant `ident_t` values, which are > > problematic. They might actually be useful for the original purpose of > > `ident_t`, namely location information. Think function merging that will > > cause a call with one of multiple different `ident_t` pointers. Making sure > > we can promote the values in that case is already much harder than checking > > if all potential values are the same boolean constant. > > > 1. This is the data duplication. > 2. Compared to the previous implementation. > 3. It allows, yes, but the compiler generates constant `ident_t`. This > structure used not only for the location information, but it used also for > other purposes. There are no problems with the code inlining and optimization > for `ident_t`s. > 1. This is the data duplication. What is? Having explicit constant boolean parameters? There is no "duplication" if they are constant and the functions are inlined. If you //really think otherwise//, I'm afraid we will not make progress here without a third opinion. > 2. Compared to the previous implementation. I do not know what the previous implementation was. I'm also unsure what the point is you are trying to make. If it is different from point 1., could you please elaborate? > 3. It allows, yes, but the compiler generates constant ident_t. This > structure used not only for the location information, but it used also for > other purposes. There are no problems with the code inlining and optimization > for ident_ts. For now, maybe. I just gave you a very plausible example of how there could be performance implications in the near future due to the indirection compared to explicit boolean parameters. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59319/new/ https://reviews.llvm.org/D59319 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
