public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Eric Botcazou <ebotcazou@adacore.com>
Cc: gcc-patches@gcc.gnu.org, Hans-Peter Nilsson <hp@bitrange.com>,
	       Alexandre Oliva <aoliva@redhat.com>
Subject: Re: [RFA:] fix PR55030, wrong code from __builtin_setjmp_receiver
Date: Tue, 27 Nov 2012 18:20:00 -0000	[thread overview]
Message-ID: <20121127182001.GD2315@tucnak.redhat.com> (raw)
In-Reply-To: <20520021.fBU7p54CN9@polaris>

On Tue, Nov 27, 2012 at 06:35:52PM +0100, Eric Botcazou wrote:
> Now I agree that the discussion exists for volatile asms.  But you have for 
> example in the unmodified cse.c:
> 
>   /* A volatile ASM invalidates everything.  */
>   if (NONJUMP_INSN_P (insn)
>       && GET_CODE (PATTERN (insn)) == ASM_OPERANDS
>       && MEM_VOLATILE_P (PATTERN (insn)))
>     flush_hash_table ();

That was weird indeed, because
  asm volatile ("" : : "r" (x));
flushed (for targets that don't add any implicit clobbers) but e.g.
  asm volatile ("" : : "r" (x) : "r23");
wouldn't (then the pattern is a parallel with ASM_OPERANDS and some
CLOBBERs).  The effect of that is that it never triggered on i?86/x86_64,
because those have the implicit fprs/flags clobbers.

I was mainly arguing with the sentence that for asm volatile, the output
constraints (did you mean outputs and clobbers?) have essentially no
meaning.  While for some optimizations perhaps it might be desirable
to treat asm volatile as full optimization barrier, I'm not sure about all
of them (i.e. it would be important to look for performance degradations
caused by that change), and for var-tracking I'd argue that asm vs. asm
volatile is completely unimportant, if the asm volatile pattern (or even
UNSPEC_VOLATILE) doesn't say it clobbers or sets hard register XYZ, then it
can't change it (well, it could but is responsible of restoring it),
similarly for memory, if it doesn't clobber "memory" and doesn't set some
memory, it can't do that.

So, do you object even to the var-tracking change (which would mean if
var-tracking found that say some variable's value lives in hard register 12, when
encountering asm volatile it would mean to forget about that even when
hard register 12 isn't clobbered by the asm, nor set)?  For now var-tracking
seems to be the most important issue, as we generate invalid debug info
there (latent before, but never hit on inline asm on x86_64/i686 except
on setjmp calls).

As for other passes, the r193802 change e.g. regresses slightly modified
pr44194-1.c testcase on x86_64,
struct ints { int a, b, c; } foo();
void bar(int a, int b);

void func() {
  struct ints s = foo();
  int x;
  asm volatile ("" : "=r" (x));
  bar(s.a, s.b);
}

 func:
 .LFB0:
-	xorl	%eax, %eax
 	subq	$24, %rsp
 .LCFI0:
+	xorl	%eax, %eax
 	call	foo
-	movq	%rax, %rcx
-	shrq	$32, %rcx
-	movq	%rcx, %rsi
-	movl	%eax, %edi
+	movq	%rax, (%rsp)
+	movl	%edx, 8(%rsp)
+	movl	4(%rsp), %esi
+	movl	(%rsp), %edi
 	addq	$24, %rsp
 .LCFI1:
 	jmp	bar

To me it looks like an unnecessary pessimization, the volatile there
I understand that just it doesn't want to be moved around (e.g. scheduled
before the foo call or after bar), that it can't be DCEd, but as it doesn't
mention it modifies memory, I don't understand why it should force some
registers to stack and back when it has no way to know the compiler would be
emitting anything like that at all.
Compare that to
  asm volatile ("" : "=r" (x) : : "memory");
in the testcase, where the r193802 commit makes no difference, the
stores/loads are done in both cases.

For CSE, I'd agree it should treat asm volatile as barriers, so for cselib.c
we probably need some additional cselib_init flag how we want to treat
volatile asms...  For UNSPEC_VOLATILE, perhaps even DSE should stay
conservative, but for var-tracking.c I still don't see a reason for that.

	Jakub

  reply	other threads:[~2012-11-27 18:20 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-12  4:49 Hans-Peter Nilsson
2012-11-12  8:28 ` Eric Botcazou
2012-11-19  3:54   ` Hans-Peter Nilsson
2012-11-19  9:35     ` Eric Botcazou
2012-11-26  3:30       ` Hans-Peter Nilsson
2012-11-27 11:44       ` Hans-Peter Nilsson
2012-11-27 12:04         ` Jakub Jelinek
2012-11-27 12:27           ` Hans-Peter Nilsson
2012-11-27 13:32           ` Jakub Jelinek
2012-11-27 17:39           ` Eric Botcazou
2012-11-27 18:20             ` Jakub Jelinek [this message]
2012-12-01  0:03               ` Eric Botcazou
2012-11-28 20:10             ` Richard Sandiford
2012-11-28 20:46             ` Richard Henderson
2012-11-28 13:44           ` Hans-Peter Nilsson
2012-11-28 13:56             ` Jakub Jelinek
2012-11-28 20:53           ` Richard Henderson
2012-11-27 19:30 Uros Bizjak
2012-11-28  9:50 ` Uros Bizjak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20121127182001.GD2315@tucnak.redhat.com \
    --to=jakub@redhat.com \
    --cc=aoliva@redhat.com \
    --cc=ebotcazou@adacore.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=hp@bitrange.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).