> Hi! > > Note, the patch has been already committed. > > On Fri, May 10, 2013 at 03:48:50PM +0200, Jan Hubicka wrote: > > Going this route you will also need to update > > > > [(set_attr "type" "rotate1") > > (set (attr "length_immediate") > > (if_then_else > > (and (match_operand 1 "const1_operand") > > (ior (match_test "TARGET_SHIFT1") > > (match_test "optimize_function_for_size_p (cfun)"))) > > (const_string "0") > > (const_string "*"))) > > (set_attr "mode" "QI")]) > > > > computing the immediate size. Why we can't canonicalize this in > > folding/simplify_rtx/combiner? I do not see much benefit allowing both > > forms of instructions in our insn streams... > > Both directions of rotation make sense for variable length rotation, and > why would the generic simplify-rtx.c code want to prefer one rotate over > the other direction for constant rotation counts? That is clearly target > specific preference.
It seems to me that it is not different from normalizing reg-10 into reg+(-10) we do for years (and for good reason). It is still target preference when use add and when sub to perform the arithmetic, but it makes sense to keep single canonical form of the expression both in RTL and Gimple. For example we may want to be able to prove that (rotate reg 31) == (rotatert reg 1) is true or (rotate reg 30) == (rotatert reg 2) is also true or cross jump both variants into one instruction. So it seems to me that canonicalizing constant rotations to get the operand into range 0....bitsize/2 makes sense in general and will make the extra special case in i386.md unnecesary. > > What we could do instead of the patch I've committed just tweak expansion > of the patterns instead, assuming it is unlikely RTL optimizations > materialize a constant rotation out of non-constant rotations from expansion > time. > > Or the length_immediate attributes could be adjusted, shouldn't be too hard > (just replace that (match_operand 1 "const1_operand") in there with > (and (match_operand 1 "const_int_operand") > (ior (match_operand 1 "const1_operand") > (match_test "INTVAL (operands[1]) == GET_MODE_BITSIZE (<MODE>mode) - > 1"))) > or similar (untested). Yep, we should either update the pattern or add canonicalization. I still think the second variant is better... Honza
