2014-10-10 20:45 GMT+04:00 Jeff Law <l...@redhat.com>: > On 10/09/14 10:54, Uros Bizjak wrote: >> >> On Thu, Oct 9, 2014 at 4:07 PM, Ilya Enkovich <enkovich....@gmail.com> >> wrote: >>> >>> It appeared I changed a semantics of BNDMK expand when replaced tree >>> operations with rtl ones. >>> >>> Original code: >>> >>> + op1 = expand_normal (fold_build2 (PLUS_EXPR, TREE_TYPE (arg1), >>> + arg1, integer_minus_one_node)); >>> + op1 = force_reg (Pmode, op1); >>> >>> Modified code: >>> >>> + op1 = expand_normal (arg1); >>> + >>> + if (!register_operand (op1, Pmode)) >>> + op1 = ix86_zero_extend_to_Pmode (op1); >>> + >>> + /* Builtin arg1 is size of block but instruction op1 should >>> + be (size - 1). */ >>> + op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx, >>> + op1, 1, OPTAB_DIRECT); >>> >>> The problem is that in the fixed version we may modify value of a pseudo >>> register into which arg1 is expanded which means incorrect value for all >>> following usages of arg1. >>> >>> Didn't reveal it early because programs surprisingly rarely hit this bug. >>> I do following change to fix it: >>> >>> op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx, >>> - op1, 1, OPTAB_DIRECT); >>> + NULL, 1, OPTAB_DIRECT); >>> >>> Similar problem was also fixed for BNDNARROW. Does it look OK? >> >> >> I'm not aware of this type of limitation, and there are quite some >> similar constructs in i386.c. It is hard to say without the testcase >> what happens, perhaps one of RTX experts (CC'd) can advise what is >> recommended here. > > The problem is the call to expand_simple_binop. > > The source (op1) and the destination (op1) are obviously the same, so its > going to clobber whatever value is in there. If there are other uses of the > original value of op1, then things aren't going to work. But I'm a little > unclear how there's be other later uses of that value. Perhaps Ilya could > comment on that.
op1 is a result of expand_normal called for SSA name. Other uses of op1 come from expand of uses of this SSA name in GIMPLE code. > > Regardless, unless there's a strong reason to do so, I'd generally recommend > passing a NULL_RTX as the target for expansions so that you always get a new > pseudo. Lots of optimizers in the RTL world work better if we don't have > pseudos with multiple assignments. By passing NULL_RTX for the target we > get that property more often. So a change like Ilya suggests (though I'd > use NULL_RTX rather than NULL) makes sense. Will replace it with NULL_RTX. Thanks, Ilya > > > > Jeff
