Re: PPC64 HTM support (was [buildbot] r201513: Invalid cast)

Re: PPC64 HTM support (was [buildbot] r201513: Invalid cast)

[David Edelsohn]

Peter,

Would you please help answer Oleg questions about case 0 in
htm_expand_builtin in rs6000.c?

Thanks, David

Speaking of GEN_FCN usage in rs6000.c.  The recently added HTM builtin
code has one interesting piece:

static rtx
htm_expand_builtin (tree exp, rtx target, bool * expandedp)
{
...
switch (nopnds)
 {
 case 0:
   pat = GEN_FCN (icode) (NULL_RTX);
   break;
 case 1:
   pat = GEN_FCN (icode) (op[0]);
   break;

The 'case 0' looks suspicious.  If the function behind the pointer
really is a zero-arg function this might or might not work depending on
the ABI.  I'm not sure what the intention here is.  I've compiled
gcc/testsuite/gcc.target/powerpc/htm-builtin-1.c to see if it ever gets
into the 'case 0' but it doesn't.
The function 'htm_init_builtins' doesn't seem to handle a 'case 0'.  I'm
confused, somebody else should have a look at this please.

Re: PPC64 HTM support (was [buildbot] r201513: Invalid cast)

[Peter Bergner]

Oleg Endo wrote:
> Speaking of GEN_FCN usage in rs6000.c.  The recently added HTM builtin
> code has one interesting piece:
> 
> static rtx
> htm_expand_builtin (tree exp, rtx target, bool * expandedp)
> {
> ...
> switch (nopnds)
>  {
>  case 0:
>    pat = GEN_FCN (icode) (NULL_RTX);
>    break;
>  case 1:
>    pat = GEN_FCN (icode) (op[0]);
>    break;
> 
> The 'case 0' looks suspicious.

Yes, the "case 0:" is not needed.  It was needed at one point
when I originally modeled this code after Andreas's s390 changes.
The code then passed in the target rtx as a separate parameter
from the operands, so nopnds was actually zero in some cases.
I realized I could simplify the code a lot by placing the
target rtx into the op[] array along with the source operands
and at that point, the "case 0:" statements became dead code
as you discovered.  I'll bootstrap and regtest the change
below and commit it as an obvious fix when that is done.

Thanks for spotting this.

Peter


Index: rs6000.c
===================================================================
--- rs6000.c	(revision 201409)
+++ rs6000.c	(working copy)
@@ -11148,9 +11148,6 @@ htm_expand_builtin (tree exp, rtx target
 
 	switch (nopnds)
 	  {
-	  case 0:
-	    pat = GEN_FCN (icode) (NULL_RTX);
-	    break;
 	  case 1:
 	    pat = GEN_FCN (icode) (op[0]);
 	    break;

Re: PPC64 HTM support (was [buildbot] r201513: Invalid cast)

[Peter Bergner]

On Tue, 2013-08-06 at 22:01 -0500, Peter Bergner wrote:
> I'll bootstrap and regtest the change
> below and commit it as an obvious fix when that is done.

Everything looked as as expected.  Committed.

Peter

Re: PPC64 HTM support (was [buildbot] r201513: Invalid cast)

[Oleg Endo]

On Tue, 2013-08-06 at 22:01 -0500, Peter Bergner wrote:
> Oleg Endo wrote:
> > Speaking of GEN_FCN usage in rs6000.c.  The recently added HTM builtin
> > code has one interesting piece:
> > 
> > static rtx
> > htm_expand_builtin (tree exp, rtx target, bool * expandedp)
> > {
> > ...
> > switch (nopnds)
> >  {
> >  case 0:
> >    pat = GEN_FCN (icode) (NULL_RTX);
> >    break;
> >  case 1:
> >    pat = GEN_FCN (icode) (op[0]);
> >    break;
> > 
> > The 'case 0' looks suspicious.
> 
> Yes, the "case 0:" is not needed.  It was needed at one point
> when I originally modeled this code after Andreas's s390 changes.
> The code then passed in the target rtx as a separate parameter
> from the operands, so nopnds was actually zero in some cases.
> I realized I could simplify the code a lot by placing the
> target rtx into the op[] array along with the source operands
> and at that point, the "case 0:" statements became dead code
> as you discovered.  I'll bootstrap and regtest the change
> below and commit it as an obvious fix when that is done.
> 
> Thanks for spotting this.

No problem.  Thanks for the explanation.
BTW please don't forget to add documentation of the builtins.
Being a non-PPC person, I had to dig and hack in the backend to get the
HTM builtins to work for the test.  I think the binutils version that I
used (2.23.1) is too old or something.  Even though I specified the
"-mhtm" option it would complain about that I need to specify the
"-mhtm" option to use the builtins...

Cheers,
Oleg