[PATCH] Stage 2 projects - AltiVec rewrite

[Paolo Bonzini]

>>@@ -5999,8 +5999,12 @@ resolve_overloaded_builtin (tree functio
>>       }
>> 
>>     default:
>>-      return NULL;
>>+      if (targetm.resolve_overloaded_builtin
>>+	  && DECL_BUILT_IN_CLASS (function) == BUILT_IN_MD)
>>+	return targetm.resolve_overloaded_builtin (function, params);
>>    
>>
>You've removed the BUILT_IN_NORMAL check and not replaced it.
>This new code is in the wrong place, since this default should
>be shadowed by the BUILT_IN_NORMAL check.
>  
>
I don't understand why resolve_overloaded_builtin should only look at 
BUILT_IN_NORMALs.  BUILT_IN_FRONTENDs will fail the if and go to the 
"return NULL_TREE" just below; non-overloaded BUILT_IN_NORMALs will do 
the same.  BUILT_IN_MDs will go to the target hook.

>>+struct altivec_builtin_types
>>    
>>
>This structure should go down with its use.
>  
>
Done.

> Indeed, I make it a habit not to pre-declare anything unless forced.

This should probably be put in the coding conventions.

>>+const struct altivec_builtin_types altivec_overloaded_builtins[] = {
>>    
>>
>Not checked.
>  
>
Luckily it is tested quite thoroughly in ops.c.

>Is it certain that ID 0 doesn't need a pointer?  You could just
>as easily make it ~ID instead...
>  
>
Good suggestion, done.

>INTEGER_TYPE only?  What about enums and bool?  Do you not want these
>being promoted?  I'd have thought INTEGRAL_TYPE_P would be more appropriate.
>  
>
Done.

>>+      /* Remove the const from the pointers to simplify the overload
>>+	 matching further down.  */
>>+      if (POINTER_TYPE_P (decl_type)
>>+	  && POINTER_TYPE_P (type)
>>+	  && TYPE_QUALS (TREE_TYPE (type)) != 0)
>>+	{
>>+          if (TYPE_READONLY (TREE_TYPE (type))
>>+	      && !TYPE_READONLY (TREE_TYPE (decl_type)))
>>+	    warning ("removing const passing argument %d "
>>+		     "to an Altivec intrinsic", n + 1);
>>    
>>
>Why are you warning?
>  
>
Because the code for warning about const-ness in c-typeck.c does not 
fire.  The fold_converts in altivec_build_resolved_builtin remove consts 
before the c-typeck.c code checks for dropped qualifiers.  And the 
AltiVec testsuite does not want you to warn for dropped volatile 
qualifiers, so you have to special case either one; I chose const.

By the way, an additional warning parameter is necessary now.

>>+  for (desc = altivec_overloaded_builtins;
>>+       desc->code && desc->code != fcode; desc++)
>>+    continue;
>>    
>>
>Given the size of the table, it would be nice to make this a binary
>search, with a small linear scan at the end to find the beginning of
>the range for fcode.
>  
>
In principle I agree, also because I did not profile this loop until 
now.  In the worst possible case, that is compiling ops.c from the 
Altivec testsuite at -O0, this takes only 1% of the compilation time.  
That is 0.05 seconds for a file that has 3731 calls to Altivec builtins, 
or 13 microseconds per call.

So, I don't think it's worth the maintainance burden of keeping the 
table in sorted order of builtin code (it is not right now, and that's 
why I am using a linear scan), but I can do it if you want.  I'd rather 
keep the table as is, and change the order of the builtin codes in 
rs6000.h in a follow-up patch together with the binary search.  I would 
rather not change the order of the table entries, because that is a) 
more error prone b) less scriptable c) gives one more patch of 
unreasonable size.

>>+  { MASK_ALTIVEC, 0, "__builtin_vec_dst", ALTIVEC_BUILTIN_VEC_DST },
>
>0 is a valid insn number; you should be using CODE_FOR_nothing.
>
It requires some care, because CODE_FOR_nothing is already used to skip 
builtins that are not supported on a machine.  But I could distinguish 
the two cases using the builtin code.

>>+      error ("unresolved overload for Altivec builtin %qs",
>>+	     IDENTIFIER_POINTER (DECL_NAME (fndecl)));
>>    
>>
>%qE, fndecl ?
>  
>
Done.

>>#define vec_vaddcuw vec_addc
>>    
>>
>At minimum these should be function macros.
>
I disagree.  We would have a problem with parenthesized arguments, since 
GCC does not implement Motorola's (a,b,c,d) syntax for vectors.  Solving 
this problem is one of the aims of this patch.

>  For C++ they should probably
>not be macros at all.
>  
>
I wanted to make every builtin appear exactly the same, as much as 
possible: in my implementation, the only exception is predicates.  I 
don't know the C++ standard enough to be sure that a template for e.g. 
vec_vaddcuw would look the same as the vec_addc builtin.

>>#define vec_step(x) __builtin_vec_step (* (__typeof__ (x) *) 0)
>>    
>>
>Using  (typeof(x)){ 0 }  would probably be friendlier to the tree
>optimizers.  With this they'll see arbitrary memory being accessed
>and not do dead store elimination etc.
>  
>
They will never see it, because vec_step is lowered before the tree 
optimizers kick in.  The advantage of this trick is that the error 
message does not expose implementation details on Altivec, which is 
another aim of this patch.

Thanks for the fast review.  Here is an updated patch, 
bootstrapped/regtested powerpc-apple-darwin (C/C++) without any regression.

Paolo
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: bonz-altivec.patch
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20050429/cc81a23a/attachment.ksh>