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