From: Tom Lane <tgl@sss.pgh.pa.us>
To: YunQiang Su <wzssyqa@gmail.com>
cc: Andres Freund <andres@anarazel.de>, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15844: MIPS: remove .set mips2 in s_lock.h to fix r6 build
In-reply-to: <19682.1561230332@sss.pgh.pa.us>
References: <15844-8f62fe7e163939b3@postgresql.org>
 <15052.1560455595@sss.pgh.pa.us>
 <CAKcpw6W2WBdTVnJSeV845V78bwOPDjhROfteX7D2o0TTU2_opA@mail.gmail.com>
 <23287.1560655935@sss.pgh.pa.us>
 <CAKcpw6W8Y56cxKJna6EsA+j2ZQ1p6DeXwMJFOOjb=baPigbV+Q@mail.gmail.com>
 <25321.1560659282@sss.pgh.pa.us>
 <CAKcpw6XYkD9Ku-0Fw6VZ6=wR030c5xCBOnTGsjLzFqdffkM8dw@mail.gmail.com>
 <22615.1560701813@sss.pgh.pa.us>
 <20190617163207.kgi5m4v2g2m252t5@alap3.anarazel.de>
 <8749.1560790222@sss.pgh.pa.us>
 <CAKcpw6VZVerXTHWSxp341GeogHQBOY9p5w3VRwGMXcxcr_hhtw@mail.gmail.com>
 <19682.1561230332@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Sat, 22 Jun 2019 15:05:32 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <27465.1561242985.0@sss.pgh.pa.us>
Date: Sat, 22 Jun 2019 18:37:07 -0400
Message-ID: <27494.1561243027@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <27465.1561242985.1@sss.pgh.pa.us>

I wrote:
> So the gcc guys aren't any more wedded than we are to the rule that
> one must not generate LL/SC on MIPS-I.  They're probably assuming the
> same thing we are, which is that if you're actually doing this on
> MIPS-I then you'll get an instruction trap and the kernel will
> emulate it for you.

Actually, after further contemplating that argument (which I was reminded
of by excavating in our commit log), I realize that having a .set mips2
in the ASM is actually the right way to do this, because that only results
in LL/SC getting emulated when we need them to be.  Forcing -march=mips2
across the entire Postgres build would exceed our authority really ---
yeah, it might produce better code quality, but it's not our business to
override platform-level target architecture decisions.

So I now think your original proposal was about the best way to do this,
though I'd tweak it to make the test be "#if __mips_isa_rev < 2", as
attached.  It doesn't seem like forcing the ISA level down is ever
something we want to do, even if we know there's no practical difference.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="avoid-asm-set-mips2-2.patch";
	charset="us-ascii"
Content-ID: <27465.1561242985.2@sss.pgh.pa.us>
Content-Description: avoid-asm-set-mips2-2.patch

diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index a9a92de..d2c707e 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -598,13 +598,31 @@ tas(volatile slock_t *lock)
 
 
 #if defined(__mips__) && !defined(__sgi)	/* non-SGI MIPS */
-/* Note: R10000 processors require a separate SYNC */
 #define HAS_TEST_AND_SET
 
 typedef unsigned int slock_t;
 
 #define TAS(lock) tas(lock)
 
+/*
+ * Original MIPS-I processors lacked the LL/SC instructions, but if we are
+ * so unfortunate as to be running on one of those, we expect that the kernel
+ * will handle the illegal-instruction traps and emulate them for us.  On
+ * anything newer (and really, MIPS-I is extinct) LL/SC is the only sane
+ * choice because any other synchronization method must involve a kernel
+ * call.  Unfortunately, many toolchains still default to MIPS-I as the
+ * codegen target, so we have to be prepared to force the assembler to
+ * accept LL/SC.
+ *
+ * R10000 and up processors require a separate SYNC, which has the same
+ * issues as LL/SC.
+ */
+#if __mips_isa_rev < 2
+#define MIPS_SET_MIPS2	"       .set mips2          \n"
+#else
+#define MIPS_SET_MIPS2
+#endif
+
 static __inline__ int
 tas(volatile slock_t *lock)
 {
@@ -614,7 +632,7 @@ tas(volatile slock_t *lock)
 
 	__asm__ __volatile__(
 		"       .set push           \n"
-		"       .set mips2          \n"
+		MIPS_SET_MIPS2
 		"       .set noreorder      \n"
 		"       .set nomacro        \n"
 		"       ll      %0, %2      \n"
@@ -636,7 +654,7 @@ do \
 { \
 	__asm__ __volatile__( \
 		"       .set push           \n" \
-		"       .set mips2          \n" \
+		MIPS_SET_MIPS2 \
 		"       .set noreorder      \n" \
 		"       .set nomacro        \n" \
 		"       sync                \n" \

------- =_aaaaaaaaaa0--