[PATCH] new ".serialize" gas directive

[PATCH] new ".serialize" gas directive

[David Mosberger]

Hi HJ,

I finally got tired of those spurious dependency-violation warnings
which occur when compiling the Linux kernel.  I believe you're
planning to fix some of the DV problems in other ways, but a few of
the spurious warnings are difficult to handle even with a perfect
assembler.  For example, the perfmon code writes to a data-breakpoint
register (without "srlz.d") and then GAS complains about all
subsequent stores.  The complaint is valid _unless_ you know that the
breakpoint isn't actually enabled.  So rather than trying to have all
kinds of complicated logic to handle these special cases, I decided to
add ".serialize.instruction" and ".serialize.data" directives which,
for DV-purposes, behave exactly like:

	;; srlz.i ;;
and

	;; srlz.d ;;

respectively, except that they don't actually insert any stop-bits or
(slow) serialization instructions.

After applying the attached patch to GAS, I had to add about eight
.serialize.data directives to the kernel and now there are no more
spurious warnings except for this one:

{standard input}: Assembler messages:
{standard input}:6117: Warning: Use of 'mov' may violate WAW dependency 'GR%, %
in 1 - 127' (impliedf), specific resource number is 14
{standard input}:6117: Warning: Only the first path encountering the conflict is
 reported
{standard input}:6116: Warning: This is the location of the conflicting usage

This one comes from plain C code (mm/vmscan.c):

	tbit.z p34, p35 = r22, 12	;;
	(p34) cmp4.eq p6, p7 = 2, r15
	(p35) cmp4.eq p6, p7 = 3, r15	;;
	(p6) addl r14 = 1, r0
	(p7) mov r14 = r0

I'm not sure whether GCC ought to be emitting .pred.rel.mutex p6,p7 or
whether GAS is supposed to be smart enough to figure this out on its
own.

In any case, if you think this patch is acceptable, would you mind
checking it into binutils?

Thanks,

	--david

ChangeLog

2004-05-05  David Mosberger-Tang  <davidm@hpl.hp.com>

	* config/tc-ia64.c (dot_serialize): Declare.
	(dot_serialize): New function.
	(md_pseudo_table): Add ".serialize.data" and
	".serialize.instruction" directives.

Index: config/tc-ia64.c
=================================RCS file: /cvs/src/src/gas/config/tc-ia64.c,v
retrieving revision 1.108
diff -u -r1.108 tc-ia64.c
--- config/tc-ia64.c	4 May 2004 14:58:11 -0000	1.108
+++ config/tc-ia64.c	6 May 2004 05:01:49 -0000
@@ -750,6 +750,7 @@
 static void print_prmask PARAMS ((valueT mask));
 static void dot_pred_rel PARAMS ((int));
 static void dot_reg_val PARAMS ((int));
+static void dot_serialize PARAMS ((int));
 static void dot_dv_mode PARAMS ((int));
 static void dot_entry PARAMS ((int));
 static void dot_mem_offset PARAMS ((int));
@@ -4651,6 +4652,23 @@
   demand_empty_rest_of_line ();
 }
 
+/*
+  .serialize.data
+  .serialize.instruction
+ */
+static void
+dot_serialize (type)
+     int type;
+{
+  insn_group_break (0, 0, 0);
+  if (type)
+    instruction_serialization ();
+  else
+    data_serialization ();
+  insn_group_break (0, 0, 0);
+  demand_empty_rest_of_line ();
+}
+
 /* select dv checking mode
    .auto
    .explicit
@@ -5033,6 +5051,8 @@
     { "pred.rel.mutex", dot_pred_rel, 'm' },
     { "pred.safe_across_calls", dot_pred_rel, 's' },
     { "reg.val", dot_reg_val, 0 },
+    { "serialize.data", dot_serialize, 0 },
+    { "serialize.instruction", dot_serialize, 1 },
     { "auto", dot_dv_mode, 'a' },
     { "explicit", dot_dv_mode, 'e' },
     { "default", dot_dv_mode, 'd' },

Re: [PATCH] new ".serialize" gas directive

[Jim Wilson]

On Wed, 2004-05-05 at 22:15, David Mosberger wrote:
> 	tbit.z p34, p35 = r22, 12	;;
> 	(p34) cmp4.eq p6, p7 = 2, r15
> 	(p35) cmp4.eq p6, p7 = 3, r15	;;
> 	(p6) addl r14 = 1, r0
> 	(p7) mov r14 = r0

This one has complications.  If r22 has the NaT bit set, then the result
of the tbit.z is to set both p34 and p35 to zero.  Thus p6, p7 are not
set, and it is possible that they both could be true if we have no
information about them, in which case there is a DV here.  If we know
that r22 can not have the NaT bit set, then one and only one of p34 or
p35 will be true in which case at most one of p6 or p7 can be true, in
which case there is no DV.  So resolving this needs extra information
about NaT bits and/or the previous values of p6/p7.  I am not sure if
gas can get this right.  It may not have enough info.  This may also be
a gcc bug, since this code isn't safe if r22 can have a NaT bit set, and
I seriously doubt that gcc checks for this case.

At the moment, neither gcc nor gas really knows anything about NaT bits.

> 	* config/tc-ia64.c (dot_serialize): Declare.
> 	(dot_serialize): New function.
> 	(md_pseudo_table): Add ".serialize.data" and
> 	".serialize.instruction" directives.

The patch looks fine to me.  I checked it in.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: [PATCH] new ".serialize" gas directive

[David Mosberger]

>>>>> On 06 May 2004 16:33:36 -0700, Jim Wilson <wilson@specifixinc.com> said:

  Jim> On Wed, 2004-05-05 at 22:15, David Mosberger wrote:
  >> tbit.z p34, p35 = r22, 12	;;
  >> (p34) cmp4.eq p6, p7 = 2, r15
  >> (p35) cmp4.eq p6, p7 = 3, r15	;;
  >> (p6) addl r14 = 1, r0
  >> (p7) mov r14 = r0

  Jim> This one has complications.  If r22 has the NaT bit set, then the result
  Jim> of the tbit.z is to set both p34 and p35 to zero.  Thus p6, p7 are not
  Jim> set, and it is possible that they both could be true if we have no
  Jim> information about them, in which case there is a DV here.

That's a good point.

  Jim> At the moment, neither gcc nor gas really knows anything about
  Jim> NaT bits.

Isn't it true that GCC never produces a NaT on its own and never uses
uninitialized registers?  If so, the code should be fine.  Perhaps GCC
should assert that p34 and p35 are mutually exclusive (effectively
asserting that r22 is not a NaT)?

  >> * config/tc-ia64.c (dot_serialize): Declare.
  >> (dot_serialize): New function.
  >> (md_pseudo_table): Add ".serialize.data" and
  >> ".serialize.instruction" directives.

  Jim> The patch looks fine to me.  I checked it in.

Cool.  Thanks a lot!

	--david

Re: [PATCH] new ".serialize" gas directive

[Jim Wilson]

On Thu, 2004-05-06 at 16:47, David Mosberger wrote:
> Isn't it true that GCC never produces a NaT on its own and never uses
> uninitialized registers?

Currently, we never intentionally produce NaTs.  However, hopefully
someday we will, so we should be careful about things like this.

I am not sure about the uninitialized register thing.  It used to be the
case that you could get writes to uninitialized registers if you had an
uninitialized variable in the source.  Of course, in that case, the
program is broken anyways.  However, I think this might have been fixed
for other non-IPF reasons.  I couldn't get an unitialized register use
with a trivial example.

>  Perhaps GCC
> should assert that p34 and p35 are mutually exclusive (effectively
> asserting that r22 is not a NaT)?

There is no trivial way to do this.  At the moment, gcc only emits the
mutex directive if a predicate register is live across a basic block. 
That is the only case that gas can't figure out on its own.

New code would have to be written to handle this case.  We might just
have to emit mutex directives for every compare which will clutter up
the assembly language.  That would be unfortunate, but maybe it is
unavoidable for now.

I don't have a lot of time for IA-64 gcc work at the moment.  It might
be useful to have a gcc bug report for this so we don't lose track of
it.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: [PATCH] new ".serialize" gas directive

[David Mosberger]

>>>>> On 07 May 2004 00:48:46 -0700, Jim Wilson <wilson@specifixinc.com> said:

  Jim> I don't have a lot of time for IA-64 gcc work at the moment.  It might
  Jim> be useful to have a gcc bug report for this so we don't lose track of
  Jim> it.

I was going to submit a bug-report, but it's difficult to provide a
reliable test-case, because other changes in the compiler can easily
make one particular warning go away, just to show up in some other
place.  For example, with the CVS gcc (pre-3.5), the warning from
mm/vmscan.c is gone but instead we get several other warnings such as
this one from net/drivers/bonding/bond_main.c:

$ as -x drivers/net/bonding/bond_main.s
drivers/net/bonding/bond_main.s: Assembler messages:
drivers/net/bonding/bond_main.s:3409: Warning: Use of 'addl' may violate WAW dependency 'GR%, % in 1 - 127' (impliedf), specific resource number is 42
drivers/net/bonding/bond_main.s:3408: Warning: This is the location of the conflicting usage

The code in question looks like this:

	cmp4.eq p12, p13 = 6, r14
	addl r28 = @ltoffx(.LC26), r1
	mov r40 = r32 ;;

  (p13) cmp.eq p16, p17 = 0, r34
	ld8.mov r41 = [r28], .LC26
  (p12) br.cond.dpnt .L1108 ;;

  (p16) addl r42 = @ltoffx(.LC28), r1
  (p17) addl r42 = @ltoffx(.LC27), r1

So, I think the best the bug report could say is "try building the
Linux kernel and look for spurious dependency-violation warnings".  Is
this worth putting in a bug-report?

	--david

Re: [PATCH] new ".serialize" gas directive

[Jim Wilson]

On Mon, 2004-05-10 at 13:16, David Mosberger wrote:
> So, I think the best the bug report could say is "try building the
> Linux kernel and look for spurious dependency-violation warnings".  Is
> this worth putting in a bug-report?

I think that is a reasonable bug report, especially considering how
close we are now to getting this right.  Having a bug report for it will
be easier for me to track than an email message in a folder I never look
at.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: [PATCH] new ".serialize" gas directive

[David Mosberger]

>>>>> On 14 May 2004 01:43:55 -0700, Jim Wilson <wilson@specifixinc.com> said:

  Jim> I think that is a reasonable bug report, especially considering
  Jim> how close we are now to getting this right.  Having a bug
  Jim> report for it will be easier for me to track than an email
  Jim> message in a folder I never look at.

I see.  So I went ahead and put it in bugzilla (see
http://gcc.gnu.org/bugzilla/show_bug.cgi?id\x15445).

Thanks,

	--david

Re: [PATCH] new ".serialize" gas directive

[Chris Wedgwood]

On Wed, May 05, 2004 at 10:15:56PM -0700, David Mosberger wrote:

> I'm not sure whether GCC ought to be emitting .pred.rel.mutex p6,p7
> or whether GAS is supposed to be smart enough to figure this out on
> its own.

I was just looking over the nested fault handler this thread came to
mind.

There is exactly that situation with '.pred.rel "mutex", p6, p7' where
gas should be able to infer this from the 'cmp.eq p6,p7 = 5,r17'
preceding this and there are no intermediate labels.

Perhaps there is a reason why gas works this way that I'm missing?



  --cw

Re: [PATCH] new ".serialize" gas directive

[Jim Wilson]

On Tue, 2004-06-15 at 18:54, Chris Wedgwood wrote:
> I was just looking over the nested fault handler this thread came to
> mind.
> There is exactly that situation with '.pred.rel "mutex", p6, p7' where
> gas should be able to infer this from the 'cmp.eq p6,p7 = 5,r17'
> preceding this and there are no intermediate labels.

I assume you are talking about the first one in arch/ia64/kernel/ivt.c.
Current gas seems to get this right without the .pred.rel.

I see that the LOAD_PHYSICAL macro expansion includes a tag, which is
kind of like a label.  Perhaps there was a problem with tags in earlier
gas versions, or perhaps earlier kernel versions used an actual label
instead of a tag here.  Use of a label here may have been necessary
before the assembler had tag support for instance, in which case the
.pred.rel would have been necessary.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: [PATCH] new ".serialize" gas directive

[Chris Wedgwood]

On Wed, Jun 16, 2004 at 02:57:00PM -0700, Jim Wilson wrote:

> I assume you are talking about the first one in
> arch/ia64/kernel/ivt.c.

Yes.

> Current gas seems to get this right without the .pred.rel.

'Current' is what these days? :)

> I see that the LOAD_PHYSICAL macro expansion includes a tag, which
> is kind of like a label.

Yeah, I guess that's true so in this case I'm not sure I can blame gas
entirely, however the scope is limited and it's not actually used
outside of the macro so gas should be able to infer something about
the program flow.

However, since apparently gas does get it right and cases like this
are uncommon I would just ignore it for now.


  --cw

Re: [PATCH] new ".serialize" gas directive

[Jim Wilson]

On Wed, 2004-06-16 at 15:04, Chris Wedgwood wrote:
> 'Current' is what these days? :)

Well, I used something from the binutils cvs tree to check this, but you
may not want to do that.  Whatever is HJ's most recent binutils release
is probably what you should try using.

> Yeah, I guess that's true so in this case I'm not sure I can blame gas
> entirely, however the scope is limited and it's not actually used
> outside of the macro so gas should be able to infer something about
> the program flow.

Tags can't be used for control flow, so that should never have been an
issue.  I believe the problem here is that a label was used before gas
had tag support, and the pred.rel was required at that time.  The
.pred.rel could have been deleted when the label was converted to a tag,
but this was missed in the conversion.

If there is a label here, then there is nothing gas can do except assume
the worst, as gas has no support for building a control flow graph.  Nor
is there any easy place to put it.  Gas is a simple two pass assembler,
one pass to read the input, and a second pass to back patch stuff that
could not be calculated during the first pass.  It isn't possible to
construct a control flow graph until after the first pass, and by then
it is too late, as we have already done all of the dependency violation
analysis during the first pass.  This could be fixed, but it would
require a major rewrite of the IA-64 gas port, and it isn't clear that
this is worth the trouble.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

RE: [PATCH] new ".serialize" gas directive

[Luck, Tony]

>I see that the LOAD_PHYSICAL macro expansion includes a tag, which is
>kind of like a label.  Perhaps there was a problem with tags in earlier
>gas versions, or perhaps earlier kernel versions used an actual label
>instead of a tag here.  Use of a label here may have been necessary
>before the assembler had tag support for instance, in which case the
>.pred.rel would have been necessary.

Yes, the first implementation of LOAD_PHYSICAL used a label
(but not because of lack of assembler support for tags, I
just didn't know about them).

-Tony

Re: [PATCH] new ".serialize" gas directive

[David Mosberger]

>>>>> On 16 Jun 2004 15:42:40 -0700, Jim Wilson <wilson@specifixinc.com> said:

  Jim> Tags can't be used for control flow, so that should never have
  Jim> been an issue.

Just nit-picking: we do use tags for the Linux kernel
exception-handling purposes nowadays.  In other words: a fault will
lead to a control-flow change which may then resume execution at a
tag.

	--david