Page MenuHomePhabricator

[compiler-rt] Disable compiler-rt on 32-bit Linux SPARC
AbandonedPublic

Authored by glaubitz on Aug 17 2021, 3:23 AM.

Details

Summary

compiler-rt uses atomic built-ins such as sync_val_compare_and_swap()
which are not available for 64-bit values on 32-bit SPARC. Since these
built-ins are considered legacy by GCC upstream and have been replaced
by the
atomic_* built-ins, there are no plans to implement a 64-bit
version of __sync_val_compare_and_swap() for 32-bit SPARC (PR c/63368).

Thus, disable compiler-rt on 32-bit SPARC for the time being which is
what the build system already does for 32-bit PowerPC where compiler-rt
isn't built either.

Diff Detail

Event Timeline

glaubitz created this revision.Aug 17 2021, 3:23 AM
glaubitz requested review of this revision.Aug 17 2021, 3:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2021, 3:23 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
ro requested changes to this revision.Aug 17 2021, 5:48 AM
ro added a subscriber: ro.

Why on earth did you abandon the original patch D98575? This patch is very similar to the last version there. It makes it much harder to review and comment on the issues already raised there.

That said, this patch is unacceptable: contrary to the title, it disables 32-bit sparc support for all targets, not just Linux. This is both wrong
(things work perfectly fine on Solaris/sparcv9) and unnecessary.

I'll make further comments in D98575 to preserve the context.

This revision now requires changes to proceed.Aug 17 2021, 5:48 AM
In D108197#2949186, @ro wrote:

Why on earth did you abandon the original patch D98575? This patch is very similar to the last version there. It makes it much harder to review and comment on the issues already raised there.

Because the discussion there was stuck and I did not receive any more feedback.

That said, this patch is unacceptable: contrary to the title, it disables 32-bit sparc support for all targets, not just Linux. This is both wrong
(things work perfectly fine on Solaris/sparcv9) and unnecessary.

Well, I can rework this to limit this change to Linux only.

ro added a comment.Aug 17 2021, 6:33 AM
In D108197#2949186, @ro wrote:

Why on earth did you abandon the original patch D98575? This patch is very similar to the last version there. It makes it much harder to review and comment on the issues already raised there.

Because the discussion there was stuck and I did not receive any more feedback.

So what? Just ping the patch like everyone else. Abandoning it and submitting effectively the same patch anew just makes work harder for the reviewers.

In my case, I just stopped responding at some point because I got the impression that my comments and questions were mostly evaded or ignored. Talking to a brick wall isn't my favourite pastime.

That said, this patch is unacceptable: contrary to the title, it disables 32-bit sparc support for all targets, not just Linux. This is both wrong
(things work perfectly fine on Solaris/sparcv9) and unnecessary.

Well, I can rework this to limit this change to Linux only.

Please wait: I suspect we can use a completely different approach (the Solaris one) on Linux, too.

In D108197#2949283, @ro wrote:

So what? Just ping the patch like everyone else. Abandoning it and submitting effectively the same patch anew just makes work harder for the reviewers.

In my case, I just stopped responding at some point because I got the impression that my comments and questions were mostly evaded or ignored. Talking to a brick wall isn't my favourite pastime.

I'm not seeing any comments from you in the previous patch unless I am missing something.

ro added a comment.Aug 17 2021, 6:42 AM
In D108197#2949283, @ro wrote:

So what? Just ping the patch like everyone else. Abandoning it and submitting effectively the same patch anew just makes work harder for the reviewers.

In my case, I just stopped responding at some point because I got the impression that my comments and questions were mostly evaded or ignored. Talking to a brick wall isn't my favourite pastime.

I'm not seeing any comments from you in the previous patch unless I am missing something.

Huh? What about this one for example?

In D108197#2949312, @ro wrote:

I'm not seeing any comments from you in the previous patch unless I am missing something.

Huh? What about this one for example?

Ah, it was hidden by the newer revision ...

glaubitz updated this revision to Diff 366971.Aug 17 2021, 11:48 AM

I have updated the patch to disable compiler-rt on Linux only.

But I'm just posting this here for the sake of completeness and I'm happy to continue the discussion in the other review thread.

vitalybuka resigned from this revision.Aug 17 2021, 2:21 PM

I don't have an opinion on SPARC 32bit support