Skip to content

Conversation

ishitatsuyuki
Copy link
Contributor

@rust-highfive
Copy link
Contributor

r? @nikomatsakis

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

@ishitatsuyuki
Copy link
Contributor Author

r? @alexcrichton

if (GlobalValue::isLocalLinkage(GVS->linkage()))
continue;
auto GUID = GVS->getOriginalName();
#if LLVM_VERSION_GE(5, 0)
if (IsLiveByGUID(Ret->Index, GUID))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is just GVS->flags().Live?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied from the relevant commit. I have no idea what's the pitfall.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's switch to GVS->flags().Live.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few reasons we should not use this construction:

  • The implementation of IsLiveByGUID goes from a GUID back to the GlobalValueSummary which is already in scope here as GVS.
  • We don't need to take WithGlobalValueDeadStripping into account, this is all happening long before anything else.

Let's switch to GVS->flags().Live

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@kennytm kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 11, 2017
@ishitatsuyuki
Copy link
Contributor Author

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Dec 12, 2017

📌 Commit dffa36c has been approved by alexcrichton

@kennytm kennytm 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2017
@bors
Copy link
Collaborator

bors commented Dec 13, 2017

⌛ Testing commit dffa36c with merge fa0ee7b8773610ad8aee58d31f23265fd31af98d...

@bors
Copy link
Collaborator

bors commented Dec 13, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Dec 13, 2017

@bors retry — http://crosstool-ng.org was down.

@kennytm
Copy link
Member

kennytm commented Dec 14, 2017

@bors rollup

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Dec 15, 2017
bors added a commit that referenced this pull request Dec 15, 2017
Rollup of 7 pull requests

- Successful merges: #46601, #46652, #46690, #46705, #46710, #46728, #46737
- Failed merges:
@bors bors merged commit dffa36c into rust-lang:master Dec 15, 2017
@ishitatsuyuki ishitatsuyuki deleted the thinlto-backport branch July 15, 2018 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants