On Tue, Nov 24, 2015 at 6:18 PM, Richard Earnshaw wrote: > On 24/11/15 09:56, Richard Earnshaw wrote: >> On 24/11/15 02:51, Bin.Cheng wrote: >>>>> The aarch64's problem is we don't define addptr3 pattern, and we don't >>>>>>> have direct insn pattern describing the "x + y << z". According to >>>>>>> gcc internal: >>>>>>> >>>>>>> ‘addptrm3’ >>>>>>> Like addm3 but is guaranteed to only be used for address calculations. >>>>>>> The expanded code is not allowed to clobber the condition code. It >>>>>>> only needs to be defined if addm3 sets the condition code. >>>>> >>>>> addm3 on aarch64 does not set the condition codes, so by this rule we >>>>> shouldn't need to define this pattern. >>> Hi Richard, >>> I think that rule has a prerequisite that backend needs to support >>> register shifted addition in addm3 pattern. >> >> addm3 is a named pattern and its format is well defined. It does not >> take a shifted operand and never has. >> >>> Apparently for AArch64, >>> addm3 only supports "reg+reg" or "reg+imm". Also we don't really >>> "does not set the condition codes" actually, because both >>> "adds_shift_imm_*" and "adds_mul_imm_*" do set the condition flags. >> >> You appear to be confusing named patterns (used by expand) with >> recognizers. Anyway, we have >> >> (define_insn "*add__" >> [(set (match_operand:GPI 0 "register_operand" "=r") >> (plus:GPI (ASHIFT:GPI (match_operand:GPI 1 "register_operand" "r") >> (match_operand:QI 2 >> "aarch64_shift_imm_" "n")) >> (match_operand:GPI 3 "register_operand" "r")))] >> >> Which is a non-flag setting add with shifted operand. >> >>> Either way I think it is another backend issue, so do you approve that >>> I commit this patch now? >> >> Not yet. I think there's something fundamental amiss here. >> >> BTW, it looks to me as though addptr3 should have exactly the same >> operand rules as add3 (documentation reads "like add3"), so a >> shifted operand shouldn't be supported there either. If that isn't the >> case then that should be clearly called out in the documentation. >> >> R. >> > > PS. > > I presume you are aware of the canonicalization rules for add? That is, > for a shift-and-add operation, the shift operand must appear first. Ie. > > (plus (shift (op, op)), op) > > not > > (plus (op, (shift (op, op)) Hi Richard, Thanks for the comments. I realized that the not-recognized insn issue is because the original patch build non-canonical expressions. When reloading address expression, LRA generates non-canonical register scaled insn, which can't be recognized by aarch64 backend. Here is the updated patch using canonical form pattern, it passes bootstrap and regression test. Well, the ivo failure still exists, but it analyzed in the original message. Is this patch OK? As for Jiong's concern about the additional extension instruction, I think this only stands for atmoic load store instructions. For general load store, AArch64 supports zext/sext in register scaling addressing mode, the additional instruction can be forward propagated into memory reference. The problem for atomic load store is AArch64 only supports direct register addressing mode. After LRA reloads address expression out of memory reference, there is no combine/fwprop optimizer to merge instructions. The problem is atomic_store's predicate doesn't match its constraint. The predicate used for atomic_store is memory_operand, while all other atomic patterns use aarch64_sync_memory_operand. I think this might be a typo. With this change, expand will not generate addressing mode requiring reload anymore. I will test another patch fixing this. Thanks, bin > > R.