Patchwork [rs6000,libitm] Enable Hardware Transactional Memory (HTM) support on Power

login
register
mail settings
Submitter Jakub Jelinek
Date July 15, 2013, 7:13 p.m.
Message ID <20130715191359.GQ2475@laptop.redhat.com>
Download mbox | patch
Permalink /patch/259172/
State New
Headers show

Comments

Jakub Jelinek - July 15, 2013, 7:13 p.m.
On Mon, Jul 15, 2013 at 08:44:12PM +0200, Jakub Jelinek wrote:
> On Mon, Jul 15, 2013 at 12:25:58PM -0500, Peter Bergner wrote:
> > On Mon, 2013-07-15 at 11:55 -0400, David Edelsohn wrote:
> > > Thanks for the explanation.  Leave it in.
> > > 
> > > Jakub was asking about the status of the HTM patch and support, so
> > > please check it in.
> > 
> > Ok, committed to mainline (after another bootstrap) as revision 200960.
> > Thanks!
> 
> Looking at the patch, you've changed FIRST_PSEUDO_REGISTER (internal gcc
> thing, so fine), but that also changed DWARF_FRAME_REGISTER which was
> computed based on FIRST_PSEUDO_REGISTER.  Isn't that an ABI break in the
> unwinder (think about having parts of unwinder linked from different
> versions of libgcc)?
> Can't we just tweak DWARF_FRAME_REGISTER not to include those registers?

I'd say something like (but, untested and can't test it right now (and no
access to power8 anyway)):

2013-07-15  Jakub Jelinek  <jakub@redhat.com>

	* config/rs6000/rs6000.h (FIRST_PSEUDO_REGISTERS): Mention HTM
	registers in the comment.
	(DWARF_FRAME_REGISTERS): Subtract also the 3 HTM registers.



	Jakub
Peter Bergner - July 15, 2013, 7:46 p.m.
On Mon, 2013-07-15 at 21:13 +0200, Jakub Jelinek wrote:
> > Looking at the patch, you've changed FIRST_PSEUDO_REGISTER (internal gcc
> > thing, so fine), but that also changed DWARF_FRAME_REGISTER which was
> > computed based on FIRST_PSEUDO_REGISTER.  Isn't that an ABI break in the
> > unwinder (think about having parts of unwinder linked from different
> > versions of libgcc)?
> > Can't we just tweak DWARF_FRAME_REGISTER not to include those registers?

Thanks for noticing this.


> I'd say something like (but, untested and can't test it right now (and no
> access to power8 anyway)):

Do we also need to update DWARF_REG_TO_UNWIND_COLUMN similarly, which
also uses FIRST_PSEUDO_REGISTER when the dwarf reg is an SPE synthetic
register?  Or should we just break the dependence on FIRST_PSEUDO_REGISTER
altogether so this type of thing doesn't happen again?  Otherwise, we
should place a comment at the definition of FIRST_PSEUDO_REGISTER
warning of it's use in DWARF_FRAME_REGISTER.

Peter
Jakub Jelinek - July 15, 2013, 8:35 p.m.
On Mon, Jul 15, 2013 at 02:46:56PM -0500, Peter Bergner wrote:
> > I'd say something like (but, untested and can't test it right now (and no
> > access to power8 anyway)):
> 
> Do we also need to update DWARF_REG_TO_UNWIND_COLUMN similarly, which
> also uses FIRST_PSEUDO_REGISTER when the dwarf reg is an SPE synthetic
> register?

Looks like it.  Guess that "- 1" in there:
#define DWARF_REG_TO_UNWIND_COLUMN(r) \
  ((r) > 1200 ? ((r) - 1200 + FIRST_PSEUDO_REGISTER - 1) : (r))
stands for the sfp register and thus it would be "- 4".

>  Or should we just break the dependence on FIRST_PSEUDO_REGISTER
> altogether so this type of thing doesn't happen again?  Otherwise, we
> should place a comment at the definition of FIRST_PSEUDO_REGISTER
> warning of it's use in DWARF_FRAME_REGISTER.

That is surely an option too.  Or use (DWARF_FRAME_REGISTERS - 32)
in the DWARF_REG_TO_UNWIND_COLUMN definition instead of the current
FIRST_PSEUDO_REGISTER - 1, and perhaps make DWARF_FRAME_REGISTERS
just a number (independent on FIRST_PSEUDO_REGISTER).
Guess this depends on what David prefers.

	Jakub

Patch

--- gcc/config/rs6000/rs6000.h.jj	2013-07-15 21:08:02.000000000 +0200
+++ gcc/config/rs6000/rs6000.h	2013-07-15 21:11:06.846718216 +0200
@@ -892,15 +892,17 @@  enum data_align { align_abi, align_opt,
    in inline functions.
 
    Another pseudo (not included in DWARF_FRAME_REGISTERS) is soft frame
-   pointer, which is eventually eliminated in favor of SP or FP.  */
+   pointer, which is eventually eliminated in favor of SP or FP.
 
+   The 3 HTM registers aren't also included in DWARF_FRAME_REGISTERS.  */
+   
 #define FIRST_PSEUDO_REGISTER 117
 
 /* This must be included for pre gcc 3.0 glibc compatibility.  */
 #define PRE_GCC3_DWARF_FRAME_REGISTERS 77
 
 /* Add 32 dwarf columns for synthetic SPE registers.  */
-#define DWARF_FRAME_REGISTERS ((FIRST_PSEUDO_REGISTER - 1) + 32)
+#define DWARF_FRAME_REGISTERS ((FIRST_PSEUDO_REGISTER - 4) + 32)
 
 /* The SPE has an additional 32 synthetic registers, with DWARF debug
    info numbering for these registers starting at 1200.  While eh_frame