On Mon, Jun 24, 2013 at 12:31 PM, Michael Meissner
<meiss...@linux.vnet.ibm.com> wrote:
>> This really should have been a separate patch.
>
> Yes, you are right. I can separate it to be a separate patch if desired. The
> last I checked, there were still problems in moving to use LRA. It would be
> nice if we could get the switch for better testing, rather than continuing to
> use a branch. Right now my focus as been getting the initial power8 changes
> in, so it was added more because it was in the sandbox, I was working from.
We'll continue as is, but this set of patches should have been split
into more pieces with more descriptive ChangeLog entries to ease the
review process.
>
>> + /* 32-bit is not done yet. */
>> + if (TARGET_ELF && !TARGET_POWERPC64)
>> + return 0;
>>
>> What does "32-bit is not done yet." mean? This means PPC32 Linux is
>> not supported but PPC32 AIX is supported?
>
> I don't believe AIX and Linux 64-bit small code model will work with fusion
> loading the GPRs, except in the case where you have more than 64K in the
> static
> area that the section anchors point to. It would work with the VSX fusion
> that
> loads a small constant plus doing hte load. I tend to feel that restructuring
> the code to allow more general addresses before reload, and have secondary
> reload, generate the appropriate instructions will work better, but that may
> take a longer period to get correct (I'm starting work on it now).
>
> I hadn't gotten around to to looking at 32-bit ELF/Linux. In theory, 32-bit
> Linux should work well with fusion for non-pic code.
My question is what will break, not what will remain unoptimized. The
comment is not clear and the code addresses only avoids one very
specific target.
>> + if (TARGET_ELF && !TARGET_POWERPC64)
>> + return 0;
>>
>> Please return "true" and "false" from new predicates, not "1" and "0".
>
> Ok, I was just being constant with the existing code.
Some code uses 0/1 and some uses true/false. Newer code uses true/false.
>
>> +
>> + case DImode:
>> + if (TARGET_POWERPC64)
>> + {
>> + mode_name = "long";
>> + load_str = "ld";
>> + }
>> + break;
>>
>> What happens for DImode when not TARGET_POWERPC64? This should be
>> gcc_unreachable()?
>
> There is a gcc_unreachable () at the end of the switch that is reached by
> either an unknown mode (default case), or by DImode on 32-bit. But I can put
> in two separate gcc_unreachable ()'s.
The current implementation seems like a rather obtuse error path. If
PPC32 DImode is not supported, it would be clearer to fail there, as
opposed to the function called with a garbage or illegal mode.
Thanks, David
