Fix a few AMDGPU related issues #52514

DiamondLovesYou · 2018-07-19T03:06:44Z

AMDGPU ignores noinline and sadly doesn't clear the attribute when it slaps alwaysinline on everything,
an AMDGPU related load bit range metadata assertion,
I didn't enable the amdgpu component in the librustc_llvm build script,
Add AMDGPU call abi info.

rust-highfive · 2018-07-19T03:06:47Z

r? @eddyb

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

pietroalbini · 2018-07-23T12:00:39Z

Ping from triage @eddyb! This PR needs your review.

pietroalbini · 2018-07-30T17:13:20Z

Ping from triage! This PR needs a review, can @eddyb or someone else from @rust-lang/compiler review this?

estebank · 2018-07-30T17:25:37Z

The code itself looks good to me. Leaving to somebody else that has better knowledge of llvm to chime in and r+.

nagisa · 2018-07-30T18:04:35Z

src/librustc_target/spec/mod.rs

+    /// and basically inline everything regardless of our wishes anyway.
+    /// This causes LLVM validation errors due to the conflicting attributes.
+    /// Defaults to false.
+    pub ignore_inline_never: bool,


General strategy is to have is_like_platform for platform-specific handling.

This seems fine to me, though.

Please don't do this, if it's a LLVM-specific temporary hack, add code for it checking e.g. architecture name in triple, to rustc_codegen_llvm.

nagisa · 2018-07-30T18:11:19Z

src/librustc_codegen_llvm/builder.rs

@@ -564,6 +564,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {


    pub fn range_metadata(&self, load: ValueRef, range: Range<u128>) {
+        if self.sess().target.target.arch == "amdgpu" {
+            // amdgpu/LLVM does something weird and thinks a i64 value is


This sounds like a bug in LLVM. Consider filling a (rust-lang) issue and referencing this piece of code there.

It is definitely desirable to maintain range metadata somehow.

nagisa · 2018-07-30T18:11:45Z

@bors r+

bors · 2018-07-30T18:11:46Z

📌 Commit d992a94 has been approved by nagisa

Fix a few AMDGPU related issues * AMDGPU ignores `noinline` and sadly doesn't clear the attribute when it slaps `alwaysinline` on everything, * an AMDGPU related load bit range metadata assertion, * I didn't enable the `amdgpu` component in the `librustc_llvm` build script, * Add AMDGPU call abi info.

eddyb · 2018-07-31T07:01:30Z

src/librustc_codegen_llvm/builder.rs

@@ -564,6 +564,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {


    pub fn range_metadata(&self, load: ValueRef, range: Range<u128>) {
+        if self.sess().target.target.arch == "amdgpu" {


Basically use a check like this instead of ignore_inline_never. Both are LLVM bugs AFAICT.

eddyb · 2018-07-31T07:01:43Z

@bors r-

XAMPPRocky · 2018-08-10T15:53:39Z

Triage: @DiamondLovesYou We haven't heard from you, Are you able to get back to this?

DiamondLovesYou · 2018-08-11T18:04:25Z

Sorry, I've been out of the States for a while. I've returned but won't be all the way back home till Sunday.

XAMPPRocky · 2018-08-21T13:30:20Z

Triage: @DiamondLovesYou Hope your trip went well. Do you have any status updates on this PR?

Allow target specs to disable that attribute.

DiamondLovesYou · 2018-08-23T21:39:23Z

Ready for the second round of reviews. @eddyb @estebank

pietroalbini · 2018-08-27T15:19:49Z

Ping from triage @eddyb! This PR needs your review.

TimNN · 2018-09-04T17:32:06Z

Ping from triage @eddyb / @estebank / @rust-lang/compiler: This PR requires your review.

estebank · 2018-09-05T16:05:15Z

LGTM but deferring to @eddyb.

TimNN · 2018-09-11T17:52:15Z

Ping from triage @eddyb / @rust-lang/compiler: This PR requires your review.

eddyb · 2018-09-11T18:19:49Z

Sorry about the delay, was caught up in edition-related changes.

@bors r+

bors · 2018-09-11T18:19:50Z

📌 Commit 66e8e19 has been approved by eddyb

Fix a few AMDGPU related issues * AMDGPU ignores `noinline` and sadly doesn't clear the attribute when it slaps `alwaysinline` on everything, * an AMDGPU related load bit range metadata assertion, * I didn't enable the `amdgpu` component in the `librustc_llvm` build script, * Add AMDGPU call abi info.

@ghost

Rollup of 15 pull requests Successful merges: - #52514 (Fix a few AMDGPU related issues) - #53703 (Document .0 to unpack integer from Wrapping) - #53777 (Implemented map_or_else for Result<T, E>) - #54031 (A few cleanups and minor improvements to rustc_passes) - #54046 (Update documentation for fill_buf in std::io::BufRead) - #54064 (`&CStr`, not `CStr`, is the counterpart of `&str`) - #54072 (Stabilization change for mod.rs) - #54073 (docs: Use dollar sign for all bash prompts) - #54074 (simplify ordering for Kind) - #54085 (Remove documentation about proc_macro being bare-bones) - #54087 (rustdoc: Remove generated blanket impls from trait pages) - #54106 (Reexport CheckLintNameResult) - #54107 (Fix typos in libstd hash map) - #54136 (Update LLVM to fix GlobalISel dbg.declare) - #54142 (Recover proper regression test for issue #16278.) Failed merges: r? @ghost

bors · 2018-09-12T11:27:42Z

⌛ Testing commit 66e8e19 with merge 8586ec6...

rust-highfive assigned eddyb Jul 19, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2018

pietroalbini added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 30, 2018

nagisa reviewed Jul 30, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2018

Mark-Simulacrum mentioned this pull request Jul 30, 2018

Rollup of 22 pull requests #52882

Closed

eddyb reviewed Jul 31, 2018

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 31, 2018

DiamondLovesYou added 2 commits August 20, 2018 16:26

AMDGPU call abi info.

6ff72a1

Actually enable the amdgpu component if present.

1c0603e

DiamondLovesYou added 2 commits August 23, 2018 14:30

AMDGPU ignores noinline when it slaps alwaysinline everywhere.

bfddede

Allow target specs to disable that attribute.

Fix an AMDGPU related load bit range metadata assertion.

66e8e19

DiamondLovesYou force-pushed the amdgpu-fixes branch from d992a94 to 66e8e19 Compare August 23, 2018 19:31

pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 27, 2018

pietroalbini removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 27, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2018

kennytm mentioned this pull request Sep 12, 2018

Rollup of 15 pull requests #54146

Merged

bors merged commit 66e8e19 into rust-lang:master Sep 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a few AMDGPU related issues #52514

Fix a few AMDGPU related issues #52514

DiamondLovesYou commented Jul 19, 2018

rust-highfive commented Jul 19, 2018

pietroalbini commented Jul 23, 2018

pietroalbini commented Jul 30, 2018

estebank commented Jul 30, 2018

nagisa Jul 30, 2018

nagisa Jul 30, 2018

eddyb Jul 31, 2018

nagisa Jul 30, 2018

nagisa commented Jul 30, 2018

bors commented Jul 30, 2018

eddyb Jul 31, 2018

eddyb commented Jul 31, 2018

XAMPPRocky commented Aug 10, 2018

DiamondLovesYou commented Aug 11, 2018

XAMPPRocky commented Aug 21, 2018

DiamondLovesYou commented Aug 23, 2018

pietroalbini commented Aug 27, 2018

TimNN commented Sep 4, 2018

estebank commented Sep 5, 2018

TimNN commented Sep 11, 2018

eddyb commented Sep 11, 2018

bors commented Sep 11, 2018

bors commented Sep 12, 2018

		@@ -564,6 +564,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {


		pub fn range_metadata(&self, load: ValueRef, range: Range<u128>) {
		if self.sess().target.target.arch == "amdgpu" {

Fix a few AMDGPU related issues #52514

Fix a few AMDGPU related issues #52514

Conversation

DiamondLovesYou commented Jul 19, 2018

rust-highfive commented Jul 19, 2018

pietroalbini commented Jul 23, 2018

pietroalbini commented Jul 30, 2018

estebank commented Jul 30, 2018

nagisa Jul 30, 2018

Choose a reason for hiding this comment

nagisa Jul 30, 2018

Choose a reason for hiding this comment

eddyb Jul 31, 2018

Choose a reason for hiding this comment

nagisa Jul 30, 2018

Choose a reason for hiding this comment

nagisa commented Jul 30, 2018

bors commented Jul 30, 2018

eddyb Jul 31, 2018

Choose a reason for hiding this comment

eddyb commented Jul 31, 2018

XAMPPRocky commented Aug 10, 2018

DiamondLovesYou commented Aug 11, 2018

XAMPPRocky commented Aug 21, 2018

DiamondLovesYou commented Aug 23, 2018

pietroalbini commented Aug 27, 2018

TimNN commented Sep 4, 2018

estebank commented Sep 5, 2018

TimNN commented Sep 11, 2018

eddyb commented Sep 11, 2018

bors commented Sep 11, 2018

bors commented Sep 12, 2018