MTVU: Allow stacking of LABEL

pcsx2/Gif_Unit.cpp

@@ -142,9 +142,19 @@ bool Gif_HandlerAD_MTVU(u8* pMem)
 	else if (reg == 0x62)
 	{ // LABEL
 		GUNIT_WARN("GIF Handler - LABEL");
-		if (vu1Thread.gsLabel.load(std::memory_order_acquire) & VU_Thread::InterruptFlagLabel)
+		// It's okay to coalesce label updates
-			Console.Error("GIF Handler MTVU - Double LABEL Not Handled");
+		u32 labelData = data[0];
-		vu1Thread.gsLabel.store(((u64)data[1] << 32) | data[0], std::memory_order_relaxed);
+		u32 labelMsk = data[1];
+		u64 existing = 0;
+		u64 wanted = ((u64)labelMsk << 32) | labelData;
+		while (!vu1Thread.gsLabel.compare_exchange_weak(existing, wanted, std::memory_order_relaxed))
+		{
+			u32 existingData = (u32)existing;
+			u32 existingMsk = (u32)(existing >> 32);
+			u32 wantedData = (existingData & ~labelMsk) | (labelData & labelMsk);
+			u32 wantedMsk = existingMsk | labelMsk;
+			wanted = ((u64)wantedMsk << 32) | wantedData;
+		}
 		vu1Thread.gsInterrupts.fetch_or(VU_Thread::InterruptFlagLabel, std::memory_order_release);
 	}
 	else if (reg >= 0x63 && reg != 0x7f)

pcsx2/MTVU.cpp

@@ -337,24 +337,13 @@ void VU_Thread::Get_GSChanges()
 	if (!interrupts)
 		return;
 	
-	// We don't support stacking multiple of the same type of interrupt, so the faster we read the required data and clear the flag the less likely we'll run into issues with that
-	u64 signal, label;
-	if (interrupts & (InterruptFlagSignal | InterruptFlagLabel))
-	{
-		// label and signal access other variables so the load must have acquire semantics
-		std::atomic_thread_fence(std::memory_order_acquire);
-		signal = gsSignal.load(std::memory_order_relaxed);
-		label = gsLabel.load(std::memory_order_relaxed);
-		// Also need release semantics on the clear
-		gsInterrupts.fetch_and(~interrupts, std::memory_order_release);
-	}
-	else
-	{
-		gsInterrupts.fetch_and(~interrupts, std::memory_order_relaxed);
-	}
-	
 	if (interrupts & InterruptFlagSignal)
 	{
+		std::atomic_thread_fence(std::memory_order_acquire);
+		const u64 signal = gsSignal.load(std::memory_order_relaxed);
+		// If load of signal was moved after clearing the flag, the other thread could write a new value before we load without noticing the double signal
+		// Prevent that with release semantics
+		gsInterrupts.fetch_and(~InterruptFlagSignal, std::memory_order_release);
 		GUNIT_WARN("SIGNAL firing");
 		const u32 signalMsk = (u32)(signal >> 32);
 		const u32 signalData = (u32)signal;
@@ -377,6 +366,7 @@ void VU_Thread::Get_GSChanges()
 	}
 	if (interrupts & InterruptFlagFinish)
 	{
+		gsInterrupts.fetch_and(~InterruptFlagFinish, std::memory_order_relaxed);
 		GUNIT_WARN("Finish firing");
 		CSRreg.FINISH = true;
 		gifUnit.gsFINISH.gsFINISHFired = false;
@@ -386,6 +376,11 @@ void VU_Thread::Get_GSChanges()
 	}
 	if (interrupts & InterruptFlagLabel)
 	{
+		gsInterrupts.fetch_and(~InterruptFlagLabel, std::memory_order_acquire);
+		// If other thread updates gsLabel for a second interrupt, that's okay.  Worst case we think there's a label interrupt but gsLabel is 0
+		// We do not want the exchange of gsLabel to move ahead of clearing the flag, or the other thread could add more work before we clear the flag, resulting in an update with the flag unset
+		// acquire semantics should supply that guarantee
+		const u64 label = gsLabel.exchange(0, std::memory_order_relaxed);
 		GUNIT_WARN("LABEL firing");
 		const u32 labelMsk = (u32)(label >> 32);
 		const u32 labelData = (u32)label;