ABataev 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, ---------------- 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. 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
