diff mbox

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

Message ID 1375844473.5240.111.camel@otta
State New
Headers show

Commit Message

Peter Bergner Aug. 7, 2013, 3:01 a.m. UTC 
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

Comments

Peter Bergner Aug. 7, 2013, 1:23 p.m. UTC | #1 
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
Oleg Endo Aug. 7, 2013, 7:32 p.m. UTC | #2 
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
diff mbox

Patch

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;