Richard Biener <rguent...@suse.de> writes: > On Fri, 28 Sep 2018, David Malcolm wrote: >> This is v3 of the patch; previous versions were: >> v2: https://gcc.gnu.org/ml/gcc-patches/2018-07/msg00446.html >> v1: https://gcc.gnu.org/ml/gcc-patches/2018-06/msg01462.html >> >> This patch introduces a class opt_problem, along with wrapper >> classes for bool (opt_result) and for pointers (e.g. opt_loop_vec_info >> for loop_vec_info). >> >> opt_problem instances are created when an optimization problem >> is encountered, but only if dump_enabled_p. They are manually >> propagated up the callstack, and are manually reported at the >> "top level" of an optimization if dumping is enabled, to give the user >> a concise summary of the problem *after* the failure is reported. >> In particular, the location of the problematic statement is >> captured and emitted, rather than just the loop's location. >> >> For example: >> >> no-vfa-vect-102.c:24:3: missed: couldn't vectorize loop >> no-vfa-vect-102.c:27:7: missed: statement clobbers memory: __asm__ > __volatile__("" : : : "memory"); >> >> Changed in v3: >> * This version bootstraps and passes regression testing (on >> x86_64-pc-linux-gnu). >> * added selftests, to exercise the opt_problem machinery >> * removed the "bool to opt_result" ctor, so that attempts to >> use e.g. return a bool from an opt_result-returning function >> will fail at compile time >> * use formatted printing within opt_problem ctor to replace the >> various dump_printf_loc calls >> * dropped i18n >> * changed the sense of vect_analyze_data_ref_dependence's return >> value (see the ChangeLog) >> * add MSG_PRIORITY_REEMITTED, so that -fopt-info can show the >> messages, without them messing up the counts in scan-tree-dump-times >> in DejaGnu tests >> >> Re Richard Sandiford's feedback on the v2 patch: >> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg00560.html >> > Since the creation of the opt_problem depends on dump_enabled_p, would >> > it make sense for the dump_printf_loc to happen automatically on >> > opt_result::failure, rather than have both? >> >> Yes; this v3 patch does that: opt_result::failure_at is passed a format >> string with variadic args. If dumping is enabled, it performs the >> equivalent of dump_printf_loc in a form that will reach dumpfiles >> (and -fopt-info-internals), stashing the dumped items in the opt_problem. >> When the opt_problem is emitted at the top-level, the message is re-emitted >> (but only for -fopt-info, not for dumpfiles, to avoid duplicates that mess >> up scan-tree-dump-times in DejaGnu tests) >> >> > I guess this is bike-shedding, but personally I'd prefer an explicit >> > test for success rather than operator bool, so that: >> > >> > opt_result foo = ...; >> > bool bar = foo; >> > >> > is ill-formed. The danger otherwise might be that we drop a useful >> > opt_problem and replace it with something more generic. E.g. the >> > opt_result form of: >> > if (!ok) >> > { >> > if (dump_enabled_p ()) >> > { >> > dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, >> > "not vectorized: relevant stmt not "); >> > dump_printf (MSG_MISSED_OPTIMIZATION, "supported: "); >> > dump_gimple_stmt (MSG_MISSED_OPTIMIZATION, TDF_SLIM, stmt, 0); >> > } >> > >> > return false; >> > } >> > >> > in vect_analyze_stmt could silently drop the information provided by >> > the subroutine if we forgot to change "ok" from "bool" to "opt_result". >> >> I didn't make that change in v3: if the function returns an opt_result, then >> the "return false;" will be a compile-time failure, alerting us to the >> problem. >> >> I guess this is a matter of style, whether explicit is better than >> implicit. Dropping the operator bool would require an explicit approach, >> with something like: >> >> // Explicit style: >> opt_result res = ...; >> if (res.failed_p ()) >> return res; >> >> and: >> >> // Explicit style: >> // It's often currently called "ok": >> opt_result ok = ...; >> if (ok.failed_p ()) >> return ok; >> >> as opposed to: >> >> // Implicit style: >> opt_result res = ...; >> if (!res) >> return res; >> >> and: >> >> // Implicit style: >> opt_result ok = ...; >> if (!ok) >> return ok; >> >> I think I went with the implicit style to minimize the lines touched by >> the patch, but I'm happy with either approach. [If we're bikeshedding, >> would renaming those "ok" to "res" be acceptable also? "ok" reads to >> me like a "success" value for a status variable, rather than the status >> variable itself; it's presumably meant to be pronounced with a rising >> interrogative as it were a question - "ok?" - but that's not visible in >> the code when reading the usage sites]. >> >> Similarly, the pointer wrappers use an implicit style: >> >> // Implicit style: >> >> opt_loop_vec_info loop_vinfo >> = vect_analyze_loop (loop, orig_loop_vinfo, &shared); >> loop->aux = loop_vinfo; >> >> if (!loop_vinfo) >> if (dump_enabled_p ()) >> if (opt_problem *problem = loop_vinfo.get_problem ()) >> { >> >> but maybe an explicit style is more readable: >> >> // Explicit style: >> >> opt_loop_vec_info opt_loop_vinfo >> = vect_analyze_loop (loop, orig_loop_vinfo, &shared); >> loop_vec_info loop_vinfo = loop_vinfo.get_pointer (); >> loop->aux = loop_vinfo >> >> if (opt_loop_vinfo.failed_p ()) >> if (dump_enabled_p ()) >> if (opt_problem *problem = loop_vinfo.get_problem ()) >> { >> >> >> How would you want the code to look? >> >> Richi: do you have an opinion here? > > I wouldn't mind about the explicit variant for the bool but > the explicit for the pointer looks odd. > > In general I agree that > > opt_result foo = x; > bool bar = x; > > is confusing but I also like the brevity that is possible > with the automatic conversions so I am biased towards that. > >> (or is that style in the patch OK as-is?) > > For me it is OK as-is. > >> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. >> >> OK for trunk? > > OK. My question on 2/3 leaves Richard time to comment.
No objection from me. Thanks, Richard
