On Fri, 2010-03-19 at 22:06 +0800, fanqifei wrote: > 1. I add movsi expander in which function foor_expand_move is used to > force the operands[1] to reg and emit the move insn. > Another change is that in the define of insn “*mov_mode_insn" in which > a condition is added to prevent a store const to mem RTL insn from > being accepted. > Are these changes necessary?
Yes, this looks correct and necessary. > 2. Is is correct to use emit_move_insn in foor_expand_move? > in mips.md, the function mips_emit_move called both emit_move_insn and > emit_move_insn_1. But I don’t quite understand the comment above the > function. This looks like the kind of thing you don't need to understand now. Just call emit_move_insn, and worry about bizarre details like this later. It isn't obvious to me why it is there either. Before reload, you can create new pseudo-regs at any time if you need to load something into a register. After reload you can't. emit_move_insn_1 assumes its operands are valid. emit_move_insn checks to see if operands are valid and if not tries to fix them. Calling emit_move_insn after reload will fail if you have an invalid operand that needs to be loaded into a new pseudo. Calling emit_move_insn_1 with invalid operands will fail a different way. It looks like the mips port is trying to do something very tricky and subtle here. If you want to understand it, you probably have to find the patch that added it. Or find a testcase where it makes a difference. > 3. My understanding of the internal flow about the issue is: > The named insn “movsi” is used to generate RTL insn list from parse > tree. The insn pattern “set mem, const” is expanded by function > foor_expand_move(). For other forms of “set” insns, the template given > in the pattern is inserted. Then the insn "*mov_mode_insn" is used to > generate assembler code. In the generation, the condition of > mov_mode_insn is checked. > I am not fully confident the understanding is correct. That seems correct. movsi is used for generating RTL. mov_mode_insn is used for matching RTL. > (define_insn "*mov_mode_insn" > [(set > (match_operand:BWD 0 "nonimmediate_operand" "=r,m,r,r,r,r,r,r,x,r") > (match_operand:BWD 1 "foor_move_source_operand" "Z,r,L,I,Q,P,ni,x,r,r"))] > "(!( > (memory_operand(operands[0], SImode) && > (foor_const_operand_f(operands[1]))) > ||(memory_operand(operands[0], HImode) && > (foor_const_operand_f(operands[1]))) > ||(memory_operand(operands[0], QImode) && > (foor_const_operand_f(operands[1]))) BWD is presumably a mode macro. You can use <BWD>mode to get the enum mode name instead of having 3 copies of the test. Checking to reject mem&&const is equivalent to checking to accept reg||reg. The latter check is the conventional one and will be faster, as register_operand does less work than memory_operand, and short-cut evaluation means we only need one register_operand call in the common case. This assumes that 'x' is some kind of register which seems likely. > predicates.md: > (define_predicate "foor_const_operand" > (match_test "foor_const_operand_f(op)")) You don't need the foor_const_operand function. You can just do (match_code "const_int") Jim
