Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Support] Refactor getN1Bits so it does not work around any g++ bug #78933

Merged
merged 1 commit into from
May 26, 2024

Conversation

AtariDreams
Copy link
Contributor

@AtariDreams AtariDreams commented Jan 22, 2024

This also folds better than the previous version as well.

Proof:
https://alive2.llvm.org/ce/z/6uwy95

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 22, 2024

@llvm/pr-subscribers-llvm-support

Author: AtariDreams (AtariDreams)

Changes

This also folds better than the previous version as well.


Full diff: https://github.com/llvm/llvm-project/pull/78933.diff

1 Files Affected:

  • (modified) llvm/include/llvm/Support/Discriminator.h (+2-5)
diff --git a/llvm/include/llvm/Support/Discriminator.h b/llvm/include/llvm/Support/Discriminator.h
index fa78cf3045de35..e2d2a5bceee4b9 100644
--- a/llvm/include/llvm/Support/Discriminator.h
+++ b/llvm/include/llvm/Support/Discriminator.h
@@ -121,12 +121,9 @@ static inline unsigned getBaseFSBitEnd() {
 }
 
 // Set bits in range of [0 .. n] to 1. Used in FS Discriminators.
-static inline unsigned getN1Bits(int N) {
-  // Work around the g++ bug that folding "(1U << (N + 1)) - 1" to 0.
-  if (N == 31)
-    return 0xFFFFFFFF;
+static inline unsigned getN1Bits(unsigned N) {
   assert((N < 32) && "N is invalid");
-  return (1U << (N + 1)) - 1;
+  return 0xFFFFFFFFU >> (31 - N);
 }
 
 } // namespace llvm

This also folds better than the previous version as well.
@AtariDreams AtariDreams merged commit bb02bf7 into llvm:main May 26, 2024
4 checks passed
@boomanaiden154
Copy link
Contributor

@AtariDreams It doesn't appear like this PR (or #87560, #87313, #85162, or #91386) have been approved by a reviewer? The LLVM review policy technically does allow post-commit review, but that is typically de-facto reserved for code owners working in their specific area. These PRs should probably be reverted until they are adequately reviewed.

nikic added a commit that referenced this pull request May 27, 2024
…++ bug (#78933)"

This reverts commit bb02bf7.

PR merged without review.
@nikic
Copy link
Contributor

nikic commented May 27, 2024

I've reverted the changes.

@preames
Copy link
Collaborator

preames commented May 28, 2024

@AtariDreams It doesn't appear like this PR (or #87560, #87313, #85162, or #91386) have been approved by a reviewer? The LLVM review policy technically does allow post-commit review, but that is typically de-facto reserved for code owners working in their specific area. These PRs should probably be reverted until they are adequately reviewed.

I want to state this far more strongly. Landing PRs without appropriate review is a significant violation of the review policy and our developer practices. This is wildly inappropriate, and if continued would be a reason for revoking commit rights entirely.

In your case, please err strongly towards waiting on review. Only commit without pre-review if you have established trust with contributors in a given area. As an observer, it seems clear you have lost any trust you may have built, so please do not land a change without pre review at all for near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants