Change rustc_codegen_ssa's atomic_cmpxchg interface to return a pair of values

[WaffleLapkin]

Doesn't change much, but a little nicer that way.

[rustbot]

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

[GuillaumeGomez]

Seems like it's failing because of cg_gcc tests (added back in #118463). Not sure why though.

[GuillaumeGomez]

Suggested change

	( expected.to_rvalue(), success )
	(expected.to_rvalue(), success)

[Urgau]

Seems like it's failing because of cg_gcc tests (added back in #118463). Not sure why though.

@WaffleLapkin This has been fixed in #118706, it was a general issue with PR CI, nothing about your PR. Rebasing should be enough to fix this build issue.

[WaffleLapkin]

@rustbot ready

Indeed, rebase seems to have fixed ci. Thanks, @Urgau!

[cjgillot]

r=me with @antoyo's green light on the cg_gcc change.

[antoyo]

I see this removed some stuff like the alignment that might still be necessary.

@WaffleLapkin: Did you run all the rustc_codegen_gcc's test suite locally (the test.sh script)?

[WaffleLapkin]

@antoyo I don't think alignment is necessary, it was only used for the stores into the tuple temp. I did not run the tests and looking at it, they are quite cumbersome to setup, so I don't think I'll do it.

If you want to run tests yourself I suggest doing git fetch origin 0e4ccff807cce2f78ada9e3dda868bcdb419564d && git checkout 0e4ccff807cce2f78ada9e3dda868bcdb419564d. GitHub is setup in a way that you can just do that.

[antoyo]

Ok. I'll run the whole test suite locally just to make sure.

[antoyo]

There's this libcore test that is failing: atomic::bool_.
I'll investigate.

[antoyo]

This might also caused by this thing that was noted and changed:

since success contains the call to the intrinsic, it must be stored before expected so that we store expected after the call.

[antoyo]

This might also caused by this thing that was noted and changed:

since success contains the call to the intrinsic, it must be stored before expected so that we store expected after the call.

Yeah, that was it. The following code makes that test pass:

    fn atomic_cmpxchg(&mut self, dst: RValue<'gcc>, cmp: RValue<'gcc>, src: RValue<'gcc>, order: AtomicOrdering, failure_order: AtomicOrdering, weak: bool) -> (RValue<'gcc>, RValue<'gcc>) {
        let expected = self.current_func().new_local(None, cmp.get_type(), "expected");
        self.llbb().add_assignment(None, expected, cmp);
        // NOTE: gcc doesn't support a failure memory model that is stronger than the success
        // memory model.
        let order =
            if failure_order as i32 > order as i32 {
                failure_order
            }
            else {
                order
            };
        let success = self.compare_exchange(dst, expected, src, order, failure_order, weak);
 
        // NOTE: since success contains the call to the intrinsic, it must be added to the basic block before
         // expected so that we store expected after the call.
        let success_var = self.current_func().new_local(None, self.bool_type, "success");
        self.llbb().add_assignment(None, success_var, success);

        (expected.to_rvalue(), success_var.to_rvalue())
    }

I haven't ran the rest of the test suite yet.

[WaffleLapkin]

@rustbot ready

[cjgillot]

@bors r+

[bors]

📌 Commit 6cf6139 has been approved by cjgillot

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6cf6139 with merge 1a147be...

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[bors]

💔 Test failed - checks-actions

[cjgillot]

@bors retry

[bors]

⌛ Testing commit 6cf6139 with merge ddca534...

[bors]

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing ddca534 to master...

[rust-timer]

Finished benchmarking commit (ddca534): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-1.3%	[-1.7%, -1.0%]	2
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-1.3%	[-1.7%, -1.0%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.9%, 1.9%]	1
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.7%	[-0.7%, -0.7%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.951s -> 669.614s (-0.35%)
Artifact size: 311.80 MiB -> 311.77 MiB (-0.01%)