On Tue, Nov 22, 2016 at 08:25:45PM +0300, Alexander Monakov wrote: > On Fri, 11 Nov 2016, Jakub Jelinek wrote: > > Ok for trunk, once the needed corresponding config/nvptx bits are committed, > > with one nit below that needs immediate action and the rest can be resolved > > incrementally. I'd like to check in afterwards the attached patch, at least > > for now, so that non-offloaded SIMD code is less affected. > > Testing your patch revealed an issue in Fortran offloaded code; types of > boolean_type_node in f951 and boolean_false_node in lto1 (when > omp_device_lower > runs) don't match. I'm attaching a revised patch that addresses it by simply > using an integer type (there are also two other minor issues, below).
Ok. > > Please change this into > > (ENABLE_OFFLOADING && (flag_openmp || in_lto)) > > for now, so that we don't waste compile time even when clearly it > > isn't needed, and incrementally change the inliner to propagate > > the property. > > As ENABLE_OFFLOADING is not set in the offloading compiler, this additionally > needs to accept ACCEL_COMPILER. Applied like this: > > + virtual bool gate (function *ARG_UNUSED (fun)) > + { > + /* FIXME: this should use PROP_gimple_lomp_dev. */ > +#ifdef ACCEL_COMPILER > + return true; > +#else > + return ENABLE_OFFLOADING && (flag_openmp || in_lto_p); > +#endif > + } Makes sense. > > @@ -4314,6 +4364,12 @@ lower_rec_simd_input_clauses (tree new_v > > if (max_vf == 0) > > { > > max_vf = omp_max_vf (); > > + if (find_omp_clause (gimple_omp_for_clauses (ctx->stmt), > > + OMP_CLAUSE__SIMT_)) > > + { > > + int max_simt = omp_max_simt_vf (); > > + max_vf = MAX (max_vf, max_simt); > > + } > > I don't believe here there's a need to take a maximum. Cloning the loop > upfront > means that SIMD+SIMT styles are not going to mix within a single loop. I've > simplified it to an if-then-else in the revised patch. Ok. > > @@ -10601,7 +10656,11 @@ expand_omp_simd (struct omp_region *regi > > bool offloaded = cgraph_node::get (current_function_decl)->offloadable; > > for (struct omp_region *rgn = region; !offloaded && rgn; rgn = > > rgn->outer) > > offloaded = rgn->type == GIMPLE_OMP_TARGET; > > - bool is_simt = offloaded && omp_max_simt_vf () > 1 && safelen_int > 1; > > + bool is_simt > > + = (offloaded > > + && find_omp_clause (gimple_omp_for_clauses (fd->for_stmt), > > + OMP_CLAUSE__SIMT_) > > + && safelen_int > 1); > > Here computation of 'offloaded' is no longer needed, because presence of > OMP_CLAUSE__SIMT_ would imply that. Removed in the revised patch. > > I've noticed that your patch doesn't adjust 'maybe_simt' in "ordered" > lowering. > Not sure if that's intentional -- as I understand it's possible to look at the > enclosing context's clauses because 'omp ordered' must be closely nested with Right now omp ordered simd for non-simt basically causes vf 1, because the vectorizer isn't ready for having non-vectorized portions of code within vectorized loop. > the corresponding loop. I've added a FIXME in the patch. Ok for trunk, thanks. Jakub