Stream: git-cranelift

Topic: cranelift / PR #1174 Tls support for ELF and MachO

GitHub (Jan 26 2020 at 13:18):

bjorn3 edited PR #1174 from tls_support to master:

This adds TLS (thread local storage) support for ELF targets to Cranelift

• [x] ASM of the tls_example.clif file has been verifier to match the ASM emitted by LLVM at https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-531466812
• [x] Test runtime: object::write doesn't have enough TLS support yet to test execution.
• [ ] Verify encodings are correct and idiomatic.
• [x] Rustfmt
• [x] Add flag to switch between TLS styles (ELF, machO) and set it automatically from either codegen or backends.

• [ ] Add tests

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so and/or ping
  bnjbvr. <strike>The list of suggested reviewers on the right can help you.</strike> No list of suggested reviewers shown on the right side.

Fixes #963

cc @philipc https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-526909856
cc @abrown Because you implemented SIMD support for x86, I think you know enough of x86 encodings to verify that the encodings are correct.

GitHub (Feb 13 2020 at 19:52):

bjorn3 updated PR #1174 from tls_support to master:

This adds TLS (thread local storage) support for ELF targets to Cranelift

• [x] ASM of the tls_example.clif file has been verifier to match the ASM emitted by LLVM at https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-531466812
• [x] Test runtime: object::write doesn't have enough TLS support yet to test execution.
• [ ] Verify encodings are correct and idiomatic.
• [x] Rustfmt
• [x] Add flag to switch between TLS styles (ELF, machO) and set it automatically from either codegen or backends.

• [ ] Add tests

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so and/or ping
  bnjbvr. <strike>The list of suggested reviewers on the right can help you.</strike> No list of suggested reviewers shown on the right side.

Fixes #963

cc @philipc https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-526909856
cc @abrown Because you implemented SIMD support for x86, I think you know enough of x86 encodings to verify that the encodings are correct.

GitHub (Feb 14 2020 at 14:04):

bjorn3 updated PR #1174 from tls_support to master:

This adds TLS (thread local storage) support for ELF targets to Cranelift

• [x] ASM of the tls_example.clif file has been verifier to match the ASM emitted by LLVM at https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-531466812
• [x] Test runtime: object::write doesn't have enough TLS support yet to test execution.
• [ ] Verify encodings are correct and idiomatic.
• [x] Rustfmt
• [x] Add flag to switch between TLS styles (ELF, machO) and set it automatically from either codegen or backends.

• [ ] Add tests

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so and/or ping
  bnjbvr. <strike>The list of suggested reviewers on the right can help you.</strike> No list of suggested reviewers shown on the right side.

Fixes #963

cc @philipc https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-526909856
cc @abrown Because you implemented SIMD support for x86, I think you know enough of x86 encodings to verify that the encodings are correct.

GitHub (Feb 14 2020 at 15:23):

bjorn3 updated PR #1174 from tls_support to master:

This adds TLS (thread local storage) support for ELF targets to Cranelift

• [x] ASM of the tls_example.clif file has been verifier to match the ASM emitted by LLVM at https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-531466812
• [x] Test runtime: object::write doesn't have enough TLS support yet to test execution.
• [ ] Verify encodings are correct and idiomatic.
• [x] Rustfmt
• [x] Add flag to switch between TLS styles (ELF, machO) and set it automatically from either codegen or backends.

• [ ] Add tests

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so and/or ping
  bnjbvr. <strike>The list of suggested reviewers on the right can help you.</strike> No list of suggested reviewers shown on the right side.

Fixes #963

cc @philipc https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-526909856
cc @abrown Because you implemented SIMD support for x86, I think you know enough of x86 encodings to verify that the encodings are correct.

GitHub (Feb 24 2020 at 17:45):

sunfishcode submitted PR Review.

GitHub (Feb 24 2020 at 17:45):

sunfishcode submitted PR Review.

GitHub (Feb 24 2020 at 17:45):

sunfishcode created PR Review Comment:

Instead of having a tls_value instruction, would it work to just continue to use symbol_value here, and use the tls flag of gv to determine whether it's TLS or not? You could add a is_tls_data predicate function which could work the same way as is_colocated_data, FormatPredicateKind::IsColocatedData, and so on, and then define_entity_ref in cranelift-codegen/meta/src/isa/x86/encodings.rs could use that to decide when to apply the TLS legalizations.

GitHub (Feb 24 2020 at 17:45):

sunfishcode created PR Review Comment:

The ELF and Mach-O instructions here have the same signature, and essentially represent the same functionality, except that they record which target to codegen for. Would it simplify the code to just have one instruction, and move the target check into the encoding phase?

GitHub (Feb 24 2020 at 17:53):

abrown assigned PR #1174.

GitHub (Feb 24 2020 at 18:47):

bjorn3 edited PR #1174 (assigned to abrown) from tls_support to master:

This adds TLS (thread local storage) support for ELF targets to Cranelift

• [x] ASM of the tls_example.clif file has been verifier to match the ASM emitted by LLVM at https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-531466812
• [x] Test runtime: object::write doesn't have enough TLS support yet to test execution.
• [ ] Verify encodings are correct and idiomatic.
• [x] Rustfmt
• [x] Add flag to switch between TLS styles (ELF, machO) and set it automatically from either codegen or backends.

• [x] Add tests

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so and/or ping
  bnjbvr. <strike>The list of suggested reviewers on the right can help you.</strike> No list of suggested reviewers shown on the right side.

Fixes #963

cc @philipc https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-526909856
cc @abrown Because you implemented SIMD support for x86, I think you know enough of x86 encodings to verify that the encodings are correct.

GitHub (Feb 24 2020 at 18:47):

bjorn3 edited PR #1174 (assigned to abrown) from tls_support to master:

This adds TLS (thread local storage) support for ELF targets to Cranelift

• [x] ASM of the tls_example.clif file has been verifier to match the ASM emitted by LLVM at https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-531466812
• [x] Test runtime: object::write doesn't have enough TLS support yet to test execution.
• [x] Verify encodings are correct and idiomatic.
• [x] Rustfmt
• [x] Add flag to switch between TLS styles (ELF, machO) and set it automatically from either codegen or backends.

• [x] Add tests

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so and/or ping
  bnjbvr. <strike>The list of suggested reviewers on the right can help you.</strike> No list of suggested reviewers shown on the right side.

Fixes #963

cc @philipc https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-526909856
cc @abrown Because you implemented SIMD support for x86, I think you know enough of x86 encodings to verify that the encodings are correct.

GitHub (Feb 24 2020 at 19:06):

bjorn3 created PR Review Comment:

Both x86_elf_tls_get_addr and x86_macho_tls_get_addr happen to clobber a register. I am currently representing this as an extra output value. In the future there may be other TLS models added that don't clobber exactly one, but zero, two or more registers. In that case a separate instruction is needed. For symmetry I think it would be nice to keep separate instructions for ELF and Mach-O too.

GitHub (Feb 24 2020 at 19:06):

bjorn3 submitted PR Review.

GitHub (Feb 24 2020 at 19:06):

sunfishcode submitted PR Review.

GitHub (Feb 24 2020 at 19:06):

sunfishcode created PR Review Comment:

Makes sense!

GitHub (Feb 24 2020 at 19:14):

bjorn3 submitted PR Review.

GitHub (Feb 24 2020 at 19:14):

bjorn3 created PR Review Comment:

symbol_value can directly be encoded, while tls_value needs to be legalized to the correct instruction for the respective object format. See comment below for why. As far as I know, unlike an encoding, there is no way to predicate a legalization.

GitHub (Feb 25 2020 at 20:18):

bjorn3 updated PR #1174 (assigned to abrown) from tls_support to master:

This adds TLS (thread local storage) support for ELF targets to Cranelift

• [x] ASM of the tls_example.clif file has been verifier to match the ASM emitted by LLVM at https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-531466812
• [x] Test runtime: object::write doesn't have enough TLS support yet to test execution.
• [x] Verify encodings are correct and idiomatic.
• [x] Rustfmt
• [x] Add flag to switch between TLS styles (ELF, machO) and set it automatically from either codegen or backends.

• [x] Add tests

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so and/or ping
  bnjbvr. <strike>The list of suggested reviewers on the right can help you.</strike> No list of suggested reviewers shown on the right side.

Fixes #963

cc @philipc https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-526909856
cc @abrown Because you implemented SIMD support for x86, I think you know enough of x86 encodings to verify that the encodings are correct.

GitHub (Feb 25 2020 at 20:21):

bjorn3 updated PR #1174 (assigned to abrown) from tls_support to master:

This adds TLS (thread local storage) support for ELF targets to Cranelift

• [x] ASM of the tls_example.clif file has been verifier to match the ASM emitted by LLVM at https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-531466812
• [x] Test runtime: object::write doesn't have enough TLS support yet to test execution.
• [x] Verify encodings are correct and idiomatic.
• [x] Rustfmt
• [x] Add flag to switch between TLS styles (ELF, machO) and set it automatically from either codegen or backends.

• [x] Add tests

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so and/or ping
  bnjbvr. <strike>The list of suggested reviewers on the right can help you.</strike> No list of suggested reviewers shown on the right side.

Fixes #963

cc @philipc https://github.com/bjorn3/rustc_codegen_cranelift/issues/388#issuecomment-526909856
cc @abrown Because you implemented SIMD support for x86, I think you know enough of x86 encodings to verify that the encodings are correct.

GitHub (Feb 26 2020 at 01:49):

abrown submitted PR Review.

GitHub (Feb 26 2020 at 01:50):

abrown merged PR #1174.

Last updated: Oct 23 2024 at 20:03 UTC