On Tue, May 17, 2016 at 1:18 PM, Jiong Wang <jiong.w...@foss.arm.com> wrote: > On 17/05/16 11:23, Uros Bizjak wrote: >> >> On Tue, May 17, 2016 at 12:17 PM, Uros Bizjak <ubiz...@gmail.com> wrote: >>> >>> Hello! >>> >>>> This bug is introduced by my commit r236181 where the inner rtx of >>>> SUBREG haven't been checked while it should as "in_class_p" only >>>> works with REG, and SUBREG_REG is actually not always REG. If REG_P >>>> check failed, then we should fall back to normal code patch. The >>>> following simple testcase for x86 can reproduce this bug. >>>> diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c >>>> index 56ab5b4..e4e6c8c 100644 >>>> --- a/gcc/lra-constraints.c >>>> +++ b/gcc/lra-constraints.c >>>> @@ -1317,7 +1317,8 @@ process_addr_reg (rtx *loc, bool check_only_p, >>>> rtx_insn **before, rtx_insn **aft >>>> register, and this normally will be a subreg which should be reloaded >>>> as a whole. This is particularly likely to be triggered when >>>> -fno-split-wide-types specified. */ >>>> - if (in_class_p (reg, cl, &new_class) >>>> + if (!REG_P (reg) >>>> + || in_class_p (reg, cl, &new_class) >>>> || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode)) >>>> loc = &SUBREG_REG (*loc); >>> >>> Why not check SUBREG_P instead of !REG_P? >> >> Or, alternatively: >> >> if ((REG_P && !in_class_p (reg, ...)) >> || GET_MODE_SIZE ...) >> >> Which is IMO much more readable. > > > Thanks for review. > > I think your proposed rewrite will be the following, > > if (!(REG_P (reg) && !in_class_p (reg, cl, &new_class)) > || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode)) > > I feel the original code is easier to understand. > The check logic is composed of three conditions, in a pre-requisite order, > if one condition is true then reload the inner rtx, otherwise reload the > whole subreg. > > if (!REG_P (reg) > || in_class_p (reg, cl, &new_class) > || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode)) > loc = &SUBREG_REG (*loc);
Well, I don't want to bikeshed about it, it is OK with me either way. (You still need a review of functionality from Vlad, though ...) Thanks, Uros.
