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).