This is an archive of the discontinued LLVM Phabricator instance.

Move x86-specific sources to x86-specific source lists.
ClosedPublic

Authored by saugustine on Aug 9 2017, 3:00 PM.

Event Timeline

saugustine created this revision.Aug 9 2017, 3:00 PM
nemanjai edited edge metadata.Aug 10 2017, 4:51 PM

Thank you for doing this. These were causing warnings with some compilers when built on PowerPC because the sources were just empty (macro-guarded). Not compiling them at all is a much cleaner solution.
LGTM but I am far from an authority on this part of the code so I'll let the approval come from one of the other reviewers.

aheejin edited edge metadata.Aug 11 2017, 2:56 PM

Looks OK to me, but I haven't worked on this part of code either, so I think someone else should approve it.

mgorny requested changes to this revision.Aug 11 2017, 11:31 PM

Also, I think that if you're splitting them up, it'd also logical to move them into a subdirectory, x86-common maybe.

compiler-rt/lib/builtins/CMakeLists.txt
219

This and the following files have only:

#if !_ARCH_PPC

so I suppose it's currently included on more targets than x86. If it's really not useful there, I think it'd be better to update the #ifs first (and preferably get the author to review).

This revision now requires changes to proceed.Aug 11 2017, 11:31 PM

I've cleaned up this patch a bit. Now the only files that are in the x86_ARCH group are those that require 80 bits floats and cpu_model.c. Tests for all of these were already disabled on arm and powerpc (because neither has 80-bit floats), so we knew these library functions don't work on arm, but we compiled them anyway. It makes more sense to just not compile them at all if we know they require something the target architecture doesn't support.

I'm cc'ing some relevant people on this. The arm folks can comment on this patch (and I will add them to review) if there is a problem.

Also, I think that if you're splitting them up, it'd also logical to move them into a subdirectory, x86-common maybe.

I'm reluctant to do this because that would require adding another layer to the file the filtering code around line 526, and that already has some special casing and is a bit hard to follow. Nevertheless, if you insist, I will.

saugustine edited edge metadata.

Thanks for the various comments. Please take another look.

saugustine added inline comments.
compiler-rt/lib/builtins/CMakeLists.txt
219

As discussed in D36764, these all assume 80-bit floats, and therefore are only useful on x86 (and ancient 68ks)--and, in fact, the tests for these functions are disabled for everything but x86. I have left these #ifs in for now, but will remove them after this patch gets submitted (and add comments about them needing 80-bit floats). Otherwise, powerpc builds will be broken.

Remove two files inadvertantly included in last patch.

weimingz edited edge metadata.

LGTM. Adding Renato and Saleem to approve.

weimingz added inline comments.Aug 22 2017, 3:59 PM
compiler-rt/lib/builtins/CMakeLists.txt
483

why these files were not used before? should the change be in a separate patch? or update the patch title/summary as well?

mgorny added inline comments.Aug 22 2017, 9:28 PM
compiler-rt/lib/builtins/CMakeLists.txt
219

Please do not remove the guards. We're using them in other arch-specific files as well. Instead, update them to state the correct targets.

compnerd accepted this revision.Aug 22 2017, 10:16 PM

Would be nice to split up the PPC fixes into its own commit.

mgorny resigned from this revision.Aug 27 2017, 3:11 AM

I'm not going to block this but I agree with others that the PPC changes look like they belong in a separate commit.

This revision is now accepted and ready to land.Aug 27 2017, 3:11 AM
saugustine closed this revision.Nov 30 2017, 11:41 AM

Committed as R319464.