On 10/09/12 16:40, Christophe Lyon wrote: > On 7 September 2012 17:28, Richard Earnshaw <rearn...@arm.com> wrote: >> >> Ah, sigh! I'd forgotten about the cond-exec issue. That makes things >> a little awkward, since we also have to deal with the fact that thumb1 >> does not support predication. The solution, unfortunately, is thus a >> bit more involved. >> > Sorry if your suggestion makes me ask a few more questions :-) >
No problem :-) >> What we need are two patterns (although currently it looks like we've got >> two, >> in reality the predication means there are three), which need to read: > What is the advantage of the version you propose? > I mean there are already two explicit patterns, your proposal does not > really bring factorization since we end up with two patterns. > The code generated by recog would have to recoginize three possible patterns otherwise. Predication effectively goes through the insn list and generates additional patterns for each predicable insn. You never see them, but they're in there somewhere... It's relatively minor but it does lead to a slightly simpler recognizer, which should mean a smaller, faster compiler. >> (define_insn "*arm_revsh" >> [(set (match_operand:SI 0 "s_register_operand" "=l,l,r") >> (sign_extend:SI (bswap:HI (match_operand:HI 1 >> "s_register_operand" "l,l,r"))))] >> "arm_arch6" >> "@ >> revsh\t%0, %1 >> revsh%?\t%0, %1 >> revsh%?\t%0, %1" > Why do we have to keep room for the predicate here? (%?) Doesn't this > pattern match only in unconditional cases? > Because the ARM back-end has a very late conditionalizer pass that can also generate conditional execution. It very rarely kicks in these days, but if the predication rules are in there you could end up with an instruction that the compiler thought was conditionally executed being always run. That would be bad^TM. > BTW, I didn't manage to have GCC generate conditional revsh. I merely > added an "if (y)" guard before calling builtin_bswap16, but this > didn't turn into a conditional revsh. > >> [(set_attr "arch" "t1,t2,32") >> (set_attr "length" "2,2,4")] > > > >> (define_insn "*arm_revsh_cond" >> [(cond_exec (match_operator 2 "arm_comparison_operator" >> [(match_operand 3 "cc_register" "") (const_int 0)]) >> (set (match_operand:SI 0 "s_register_operand" "=l,r") >> (sign_extend:SI (bswap:HI (match_operand:HI 1 >> "s_register_operand" "l,r")))))] >> "TARGET32_BIT && arm_arch6" >> "revsh%?\t%0, %1" >> [(set_attr "arch" "t2,*") >> (set_attr "length" "2,4")]) >> >> Note that this removes the "predicable" attribute as we now handle this >> manually rather than with the auto-generation. >> >> Sorry, this has turned out to be more complex than I originally realised. > > I understand that this is also applicable to the existing arm_rev and > thumb1_rev patterns for 32 bit swaps. I'd like to understand the > rationale & implications of your proposal. > > Thanks > > Christophe. > R.
