On Thu, Mar 22, 2018 at 6:58 PM, Jim Wilson <j...@sifive.com> wrote:
> On Wed, Mar 21, 2018 at 2:45 AM, Richard Biener
> <richard.guent...@gmail.com> wrote:
>> On Tue, Mar 20, 2018 at 11:10 PM, Jim Wilson <j...@sifive.com> wrote:
>>> This fixes a wrong-code issue on RISC-V, but in theory could be a problem
>>> for
>>> any SHIFT_COUNT_TRUNCATED target.
>
>> IMHO the real issue is that SHIFT_COUNT_TRUNCATED is used for
>> optimizing away the and early. We then rely on the compiler not
>> assuming anything on the value. Like if you'd do that on GIMPLE
>> VRP would come along and second-guess you because the shift
>> value may not be too large. This combine issue sounds very similar.
>>
>> So I'm suggesting again to instead provide patterns that
>> match (shft A (and B cst))
>
> I found a message from 2013 that talks about this.
> https://gcc.gnu.org/ml/gcc-patches/2013-11/msg00169.html
> I can try looking at this.
>
> I'd question the timing though. I don't think that trying to rewrite
> shift patterns in the RISC-V port at this point in the release process
> is a good idea. I think we should fix the combine bug now with a
> simple patch, and try reworking the shift support after the gcc-8
> release.
I'm leaving the "simple" combiner patch to review by others
but for RISC-V you could simply #define SHIFT_COUNT_TRUNCATED to zero
to fix the issue. Then add patterns if it turns out to be required
to avoid regressions. For example x86 has
;; Avoid useless masking of count operand.
(define_insn_and_split "*<shift_insn><mode>3_mask"
[(set (match_operand:SWI48 0 "nonimmediate_operand")
(any_shiftrt:SWI48
(match_operand:SWI48 1 "nonimmediate_operand")
(subreg:QI
(and:SI
(match_operand:SI 2 "register_operand" "c,r")
(match_operand:SI 3 "const_int_operand")) 0)))
(clobber (reg:CC FLAGS_REG))]
"ix86_binary_operator_ok (<CODE>, <MODE>mode, operands)
&& (INTVAL (operands[3]) & (GET_MODE_BITSIZE (<MODE>mode)-1))
== GET_MODE_BITSIZE (<MODE>mode)-1
&& can_create_pseudo_p ()"
"#"
"&& 1"
[(parallel
[(set (match_dup 0)
(any_shiftrt:SWI48 (match_dup 1)
(match_dup 2)))
(clobber (reg:CC FLAGS_REG))])]
"operands[2] = gen_lowpart (QImode, operands[2]);"
[(set_attr "isa" "*,bmi2")])
not sure why it uses define_insn_and_split here. The splitter exactly
re-introduces the issue you hit with SHIFT_COUNT_TRUNCATED
simplification...
I suppose shift expanders of the x86 backends ensure QImode
shift counts here to more reliably match the above.
Richard.
> Jim
