Page MenuHomePhabricator

[LLVM][MIPS] Introduce 64 bit atomic check in CheckAtomic.cmake.
ClosedPublic

Authored by nitesh.jain on Jun 1 2016, 11:44 PM.

Details

Reviewers
chandlerc
beanz
Summary

On some target like MIPS32 we need to explicitly link atomic library for 64 bit atomic operations. This module then can be used in LLDB (http://reviews.llvm.org/D20464) or Libcxx (http://reviews.llvm.org/D16613) for explicitly link to atomic library.

Diff Detail

Repository
rL LLVM

Event Timeline

nitesh.jain retitled this revision from to [LLVM][MIPS] Introduce a CheckAtomic64 cmake module..
nitesh.jain updated this object.
nitesh.jain added a reviewer: chandlerc.
nitesh.jain set the repository for this revision to rL LLVM.
nitesh.jain added subscribers: jaydeep, bhushan, vkalintiris and 3 others.
nitesh.jain added a subscriber: llvm-commits.

Hi Chandler Carruth,

Please could you find some time to review this ?

Thanks

My own comments:

This looks fine since it removes code duplication across multiple
subprojects. Have you tested that both libcxx and lldb work correctly when
using this module as opposed to the existing ones?

This seems generally fine but:

  1. I'd love Chris B to look at this to make sure he likes it too.
  1. Pretty sure LLVM itself needs to use this. Can you update LLVM's CMake to switch to this routine?
beanz edited edge metadata.Jun 10 2016, 12:10 PM

This looks reasonable to me. Is there a reason for having this as a new CMake module instead of just adding it to CheckAtomic.cmake?

It seems to me it could be reasonable to modify CheckAtomic.cmake to test for both 32-bit and 64-bit atomics.

nitesh.jain added a comment.EditedJun 13 2016, 1:29 AM

My own comments:

This looks fine since it removes code duplication across multiple
subprojects. Have you tested that both libcxx and lldb work correctly when
using this module as opposed to the existing ones?

This module work fine with LLDB but need to check with libcxx. I will check and let you know.

This looks reasonable to me. Is there a reason for having this as a new CMake module instead of just adding it to CheckAtomic.cmake?

It seems to me it could be reasonable to modify CheckAtomic.cmake to test for both 32-bit and 64-bit atomics.

I will modify CheckAtomic.cmake to test 64-bit atomic too.

Regards,
Nitesh Jain

nitesh.jain edited edge metadata.

Added 64 bit atomic check in CheckAtomic.cmake.

My own comments:

This looks fine since it removes code duplication across multiple
subprojects. Have you tested that both libcxx and lldb work correctly when
using this module as opposed to the existing ones?

This module work fine with LLDB but need to check with libcxx. I will check and let you know.

I am able to build lldb and libcxx with this module.

-nj

Hi Chandler/beanz,

Please could you find some time to review this ?

Thanks

beanz accepted this revision.Jun 20 2016, 2:48 PM
beanz edited edge metadata.

LGTM.

Thanks!
-Chris

This revision is now accepted and ready to land.Jun 20 2016, 2:48 PM
nitesh.jain retitled this revision from [LLVM][MIPS] Introduce a CheckAtomic64 cmake module. to [LLVM][MIPS] Introduce 64 bit atomic check in CheckAtomic.cmake..Jun 20 2016, 11:10 PM
nitesh.jain edited edge metadata.
slthakur closed this revision.Jun 22 2016, 11:58 PM