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: > > > 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. 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
