On Tue, Aug 13, 2024 at 10:48 PM Jeff Law <jeffreya...@gmail.com> wrote: > > > > On 8/13/24 5:57 AM, Manolis Tsamis wrote: > > Now that more operations are allowed for noce_convert_multiple_sets, we > > need to > > check noce_can_force_operand on the sequence before calling > > try_emit_cmove_seq. > > Otherwise an inappropriate argument may be given to copy_to_mode_reg and > > result > > in an ICE. > > > > Fix-up for the recent ifcvt commit 72c9b5f438f22cca493b4e2a8a2a31ff61bf1477 > > > > PR tree-optimization/116353 > > > > gcc/ChangeLog: > > > > * ifcvt.cc (bb_ok_for_noce_convert_multiple_sets): Check > > noce_can_force_operand. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.target/i386/pr116353.c: New test. > OK. > > Note I'm not entirely sure that noce_can_force_operand is sufficient > based on what I've seen on the v8 port. > > What I'm seeing on the v8 port is that we're trying to create a > conditional move where one arm is a rotate expression. That in and of > itself isn't an issue. The port does (of course) expect a register > operand, so we use force_operand to get the result into a GPR. So far, > so good. > > The ifcvt code does check noce_can_force_operand which returns true. I > don't remember the precise details other than it looked reasonable. So > still, so far, so good. > > The problem in the v850 doesn't have a generalized rotation pattern. > The expander will FAIL for most rotate counts and there's no alternate > synthesis currently defined in the optabs interface for a word mode > rotate. So that in turn causes force_operand to return NULL_RTX to its > caller and boom! > > The rotation patterns are allowed to FAIL if I'm reading the docs > correctly. So it seems like what the v8 port is doing is valid, but it > is causing a segfault and this testsuite failure: > > > Tests that now fail, but worked before (4 tests): > > > > v850-sim/-mgcc-abi/-msoft-float/-mv850e3v5: gcc: > > gcc.c-torture/execute/20100805-1.c -O3 -fomit-frame-pointer > > -funroll-loops -fpeel-loops -ftracer -finline-functions (test for excess > > errors) > > v850-sim/-mgcc-abi/-msoft-float/-mv850e3v5: gcc: > > gcc.c-torture/execute/20100805-1.c -O3 -fomit-frame-pointer > > -funroll-loops -fpeel-loops -ftracer -finline-functions (test for excess > > errors) > > v850-sim/-msoft-float/-mv850e3v5: gcc: gcc.c-torture/execute/20100805-1.c > > -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer > > -finline-functions (test for excess errors) > > v850-sim/-mv850e3v5: gcc: gcc.c-torture/execute/20100805-1.c -O3 > > -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer > > -finline-functions (test for excess errors) > > > But perhaps it isn't that bad in practice. I can fix this by removing a > bit of what I expect is unnecessary code in the v850 port. > Hi Jeff,
Yes, what you're saying is right, testing noce_can_force_operand for SET_SRC is not sufficient. I don't think you should change the v850 backend, this should be fixed in ifcvt. As far as my analysis goes this looks to be the same issue with PR116358. We can force_operand SET_SRC just fine, but that doesn't imply we can force_operand its individual arguments. I'm testing a solution that involves checking noce_can_force_operand in try_emit_cmove_seq, so that we know if it's safe to call emit_conditional_move (essentially an extension of this initial fix). Thanks, Manolis > Jeff
