Richard Sandiford wrote:
>
> You might as well make the first operand a register_operand and
> avoid the REG_P check.
I agree. I implemented this change and it works as expected.
>More importantly:
>
>> operands[4] = simplify_gen_subreg(QImode,operands[1],HImode,0);
>> operands[5] = simplify_gen_subreg(QImode,operands[1],HImode,1);
>
> ..this code is designed to handle REGs and CONST_INTs correctly,
> and avoid the problem you were seeing. (As Eric says, gen_int_mode
> is the canonical way of generating a correct CONST_INT in cases where
> you do need to create one manually. In this case it's simpler to use
> simplify_gen_subreg though.)
I modified the code as below and now I do not have the problems I had
before.
else if (CONST_INT == GET_CODE (operands[1])
|| REG_P (operands[1])) {
operands[4] = simplify_gen_subreg(QImode,operands[1],HImode,0);
operands[5] = simplify_gen_subreg(QImode,operands[1],HImode,1);
}
> I notice you're generating subregs of symbolic constants,
> but I'm not sure that's really supported.
I did this so that I can generate rtl code that I can latter trap
with the define_insn's below.
(define_insn "movqi_hiword"
[(set (match_operand:QI 0 "register_operand" "")
(subreg:QI (match_operand:HI 1 "general_operand" "") 0))]
""
"move\t%0,#HIWORD(%1)" )
(define_insn "*movqi_lowword"
[(set (match_operand:QI 0 "register_operand" "")
(subreg:QI (match_operand:HI 1 "general_operand" "") 1))]
""
"move\t%0,#LOWWORD(%1)" )
Do you recommend a better way to do this?
> As far as the MEM_P case goes:
[...]
>
> you can use:
>
> operands[4] = adjust_address (operands[1], QImode, 1);
> operands[5] = adjust_address (operands[1], QImode, 0);
>
I also implemented this and it works nicely. Many thanks!
> The CONST handling looks suspicious here. CONSTs aren't memory references,
> but you split them into memory references.
In my debugging session, all cases I say CONSTs they were used for
memory. I will look at this in more detail.
> Also, you need to beware of cases in which operands[1] overlaps
> operands[0]. The splitter says:
>
> [(set (match_dup 2) (match_dup 4))
> (set (match_dup 3) (match_dup 5))]
>
> and operands[2] is always the highpart:
>
> operands[2] = gen_highpart(QImode, operands[0]);
>
> but consider the case in which operands[1] (and thus operands[4])
> is a memory reference that uses the high part of operands[0] as
> a base register. In that case, the base register will be modified
> by the first split instruction and have the wrong value in the
> second split instruction. See other ports for the canonical way
> of handling this.
Thanks for the heads up. Could you point me to a specific target/file?
Best regards,
-Omar
