> Regarding the implementation of popcntb, popcntd, and cmpb. Gcc has
> dedicated masks on target_flags for them, but due to shortage of bits,
> those masks controls more than the name implies.
The target flag bits control more than the name implies, but the bits
correspond to published ISA levels.
The Freescale localized parts of the patch (new scheduling
descriptions) are okay, modulo Mike's comments. We also need to
coordinate on which instructions really are part of "Altivec2" and
what name will be used for that feature.
However, what concerns me, as Mike commented as well, is this patch
adds support for a processor that does not conform to any particular
ISA -- it does not implement some instructions and adds other unique
instructions. Unlike the "SPE" instructions, these are not completely
orthogonal from the rest of the PowerPC architecture. SPE added a
separate set of SIMD instructions and merged the GPR and FPR register
sets, but left most other things alone.
The earlier addition of E500 support was a mess because the concept of
architecture and processor were not clearly defined and delineated.
This has been a maintenance nightmare for all other PowerPC
maintainers dealing with the irregularities introduced for Freescale's
non-conforming processor, especially when additional features were
added in the next processor.
At least the previous irregularities were local to Freescale's ISA
extensions. This latest processor modifies the meaning of the ISA
definitions. Changing macros that designate architectures to test for
specific processors is reverting to an approach of bad software
design. If the Freescale parts only has complete support for an
earlier ISA, that is the one it needs to use. If it implements more
"Altivec2" instructions than defined, users can access those with
inlined assembly.
Freescale can distribute compilers with whatever additional patches it
wants to include, but the cost-benefit ratio to the rest of the
PowerPC community and the rest of the GCC community is past
unreasonable.
In other words, this new processor and the latest patches mean that a
Linux distributor cannot build an application for a particular
revision of the ISA and have it work across both IBM and Freescale
processors. That is not the meaning of ISA and is going to confuse
users, developers and distros. The Freescale parts need to present
themselves as the lowest-common denominator of the processor ISA they
supports.
Thanks, David
On Fri, May 18, 2012 at 3:16 PM, Edmar <ed...@freescale.com> wrote:
> Michael,
>
> Thanks for reviewing the patch and all the suggestions.
> I have some questions / comments bellow.
>
> Regards,
> Edmar
>
>
>
> On 05/17/2012 06:16 PM, Michael Meissner wrote:
>>>
>>> In the patch I minimized the number of changes, while not adding
>>> any new mask to target_flags.
>>
>> While we may get some bits back when the original Power flags are removed,
>> it
>> may be time to bite the bullet and have two separate target flags like x86
>> did,
>> because we keep running out of bits.
>>
>
> I agree. But, wouldn't be better to have the e6500 patch separated from this
> ?
> Either way, I would like to collaborate towards having 2 target flags.
>
>> Some comments from looking at the patches:
>>
>> A meta-issue is the name of the Freescale extensions. The problem of
>> calling
>> it Altivec2 is we get into a situation like AMD and Intel have had over
>> what
>> the next SSE revision is. Perhaps using a different name than Altivec2.
>> David
>> probably needs to weigh in on this.
>
>
> That name is my fault. Internally Freescale is not giving this feature any
> new name.
> Our design manual has a table that lists the differences between the
> original
> Altivec and the current Altivec, and that is it.
>
> I thought it would be better to have independent control of the
> instructions,
> instead of just throwing everything under __ALTIVEC__
> Perhaps we should keep the control that the "-m..." option does and get rid
> of the
> __ALTIVEC2__ definition ?
>
> Regarding the spelling (-maltivec2 or other name), we do not have
> any position on it.
>
>> What is the rationale for changing modes for stv* from V4SI to V16QI, and
>> is it
>> safe? I'm just worried about existing code, that suddenly stops working.
>
>
> Understandable. Right now there is a type mismatch. The RTL is
> V4SI and the builtins are emitted with V16QI, causing an ICE.
> I traced that ICE all the way back to 4.4.
>
> BTW, the only locations that uses V4SI are the ones I changed...
>
>> In rs6000.c, I think gpopt/mfocrf should only be cleared if the user did
>> not
>> explicitly set those options. If you want to issue an error if the user
>> explicitly set the options for the new cpus, that is fine, but I don't
>> like the
>> backend to silently change options that were explicitly set on the command
>> line.
>
>
> Thanks for catching this. I will add "target_flags_explicit" to the logic.
>
>> In terms of the comments about the insns being in ISA 2.07, that may be
>> premature until the ISA is actually published. Also, some of the Altivec2
>> insns are not in the ISA 2.07 versions I've reviewed (the trouble is
>> finding
>> which insns are in which RFCs).
>
>
> Yes, although we already have silicon out to customers, this is not
> a guaranty it will be part of 2.07. I will remove explicit references to ISA
> 2.07.
>
>> I really don't like the explict CPU checks in TARGET_LFIWAX,
>> TARGET_LFIWZX,
>> because it is easy to miss when the next new cpu comes out. Perhaps it
>> would
>> be better to have one more target flag that says to ignore the
>> instructions
>> Freescale doesn't support. I dunno, or as I said, maybe it is time to
>> have two
>> target_flags. The x86 has also gone to using HOST_WIDE_INT for their two
>> sets
>> of target flags.
>
>
> I will wait for David on this.
>
>> As I mentioned earlier, I like the new popcnt type attribute, but whenever
>> you
>> add a new type attribute, you should to go through all of the *.md files
>> add
>> add support for the new attribute. Now, insns that aren't supported on
>> older
>> machines are one thing, but since popcnt has been part of the architecture
>> since the Power/PowerPC came out, I would think you need to edit
>> power4.md,
>> 7xx.md, etc. to add popcnt in the same place as integer.
>
>
> The e5500/e6500 are our first parts that does support popcntb/w/d.
> And we had manufactured 60x, 7xx, 74xx parts in the past.
> Are you sure you want popcnt on 7xx.md ?
>
> So, I have 4xx, a2, power4 through power7. Did I missed any ?
>
>> I think you need to refine the tests, and change powerpc_altivec_ok lines
>> to
>> something like:
>> /* { dg-require-effective-target powerpc_altivec2_ok } */
>>
>> And then add in support in testsuite/lib/target-supports.exp to detect
>> whether
>> the assembler supports all of the Altivec2 (or whatever we call it)
>> instructions. In the same vein, you probably need to add support in
>> configure.ac to detect whether the assembler knows about the insns similar
>> to
>> like I did in gcc_cv_as_powerpc_vsx to detect if the assembler knows about
>> VSX.
>
>
> OK, I will work on the changes.
> I will follow the name convention when it is decided (altivec2 or
> otherwise).
>
>
>
>
>
