-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[clang] Diagnose for implicit boolean conversions in assignments #114687
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
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
You can test this locally with the following command:git-clang-format --diff 8129ba6c70aef54d87893adeb34e76f84fa69fe3 b8244b346c7a97c2eb35ccfea23675b8df4f046e --extensions cpp,h -- clang/include/clang/Sema/Sema.h clang/lib/Sema/Sema.cpp clang/lib/Sema/SemaExprCXX.cppView the diff from clang-format here.diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index cc9cb173c0..934812dcc0 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -687,13 +687,13 @@ void Sema::diagnoseZeroToNullptrConversion(CastKind Kind, const Expr *E) {
<< FixItHint::CreateReplacement(E->getSourceRange(), "nullptr");
}
-void Sema::DiagnoseImplicitCastBoolAssignment(Expr* E,QualType Ty) {
+void Sema::DiagnoseImplicitCastBoolAssignment(Expr *E, QualType Ty) {
if (Ty->isBooleanType()) {
- if (BinaryOperator* Op = dyn_cast<BinaryOperator>(E)) {
+ if (BinaryOperator *Op = dyn_cast<BinaryOperator>(E)) {
// should only be issued for regular assignment `=`, not for e.g `+=`
if (Op->getOpcode() == BO_Assign) {
SourceLocation Loc = Op->getOperatorLoc();
- Diag(Loc,diag::warn_parens_bool_assign) << E->getSourceRange();
+ Diag(Loc, diag::warn_parens_bool_assign) << E->getSourceRange();
}
}
}
@@ -773,7 +773,7 @@ ExprResult Sema::ImpCastExprToType(Expr *E, QualType Ty,
}
}
- DiagnoseImplicitCastBoolAssignment(E,Ty);
+ DiagnoseImplicitCastBoolAssignment(E, Ty);
if (ImplicitCastExpr *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
if (ImpCast->getCastKind() == Kind && (!BasePath || BasePath->empty())) {
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 14ca6a7430..1edce04ed7 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -4393,7 +4393,7 @@ Sema::PerformImplicitConversion(Expr *From, QualType ToType,
FromType = From->getType();
}
- DiagnoseImplicitCastBoolAssignment(From,ToType);
+ DiagnoseImplicitCastBoolAssignment(From, ToType);
// If we're converting to an atomic type, first convert to the corresponding
// non-atomic type.
|
Identified with misc-include-cleaner.
SPARC ABI doesn't use stack realignment, so let LLVM know about it in `SparcFrameLowering`. This has the side effect of making all overaligned allocations go through `LowerDYNAMIC_STACKALLOC`, so implement the missing logic there too for overaligned allocations. This makes the SPARC backend not crash on overaligned `alloca`s and fix llvm#89569.
Identified with misc-include-cleaner.
This patch fixes: llvm/lib/Target/Sparc/SparcFrameLowering.cpp:226:29: error: unused variable 'RegInfo' [-Werror,-Wunused-variable]
Identified with misc-include-cleaner.
…4682) The value returned from the function depends only on the instruction opcode. As a drive-by, change the type of the argument to const-reference.
…14485) (llvm#114712) This time I tested on big-endian hosts.
…the header `#include <stdlib.h> (llvm#114690) This finishes the work from llvm#114453 by adding proxy headers for `malloc`, `realloc`, `free` and `aligned_alloc`.
When trying to evaluate an expression in a narrower type, the DAGCombine should propagate the disjoint flag, as it's equally valid on the narrower expression. This helps improve better use of addressing modes for some Arm SME instructions, for example.
This reverts commit b360dfd.
This reverts commit c820994.
Identified with misc-include-cleaner.
This is a follow up to llvm#111511, where after benchmarking we learnt that the Banana Pi F3 has fast segmented loads for not just NF=2, but also NF=3 and NF=4: https://github.com/preames/bp3-microarch#vlseg_lmul_x_sew_throughput This adds tuning features to allow these segment loads and stores to be costed cheaper and enables it for the spacemit-x60. It also enables +optimized-nf2-segment-load-store by default in the generic tuning to maintain the previous behaviour when compiled without -mcpu or -mtune.
…b builtins `signed char` vectors (llvm#114512) The lsxintrin.h and and lasxintrin.h headers uses `signed char` vectors instead of `unsigned char` vectors. GCC also uses `signed char` for them, so align their definition with the headers and GCC. Depends on llvm#114511. Part of llvm#110834 fix.
…ows-msvc targets (llvm#113966) Follow-up on llvm#109607, we have a use case on Windows-on-ARM64 where `cmake -G "Unix Makefiles"` generates `-fuse-ld=lld-link`, which is accidentally disallowed by PR#109607.
* As suggested in llvm#111835 (comment), ran llvm/utils/update_test_checks.py on all the copied tests. * Target triple is still unchanged --------- Co-authored-by: Kamil Kashapov <[email protected]> Co-authored-by: Vitaly Buka <[email protected]>
The scalar insert isn't the interesting bit that we're testing, so remove it to simplify the check prefixes a bit.
…backs (llvm#114547) The early simplication pipeline is used in non-LTO and (Thin/Full)LTO pre-link stage. There are some passes that we want them in non-LTO mode, but not at LTO pre-link stage. The control is missing currently. This PR adds the support. To demonstrate the use, we only enable the internalization pass in non-LTO mode for AMDGPU because having it run in pre-link stage causes some issues.
…se (llvm#114886) We have a special case where we allow the extract of the high half of a vector and consider it cheap. However, we had previously required that the type have no more than 32 elements for this to work. (Because 64/2=32, and the largest immediate for a vslidedown.vi is 31.) This has the effect of pessimizing shuffle vector lowering for long vectors - i.e. at SEW=e8, zvl128b, an m2 or m4 deinterleave can't be matched because it gets scalarized during DAG construction and can't be "profitably" rebuilt by DAG combine. Note that for RISCV, scalarization via insert and extract is extremely expensive (i.e. two vslides per element), so a slide + two half width shuffles is almost always a net win. (i.e, this isn't really specific to vnsrl) Separately, I want to look at the decision to scalarize at all, but it seems worthwhile adjusting this while we're at it regardless.
…R-V (llvm#110695) SPIR-V doesn't currently encode "native" integer bit-widths in its datalayout(s). This is problematic as it leads to optimisation passes, such as InstCombine, getting ideas and e.g. shrinking to non byte-multiple integer types, which is not desirable and can lead to breakage further down in the toolchain. This patch addresses that by encoding `i8`, `i16`, `i32` and `i64` as native types for vanilla SPIR-V (the spec natively supports them), and `i32` and `i64` for AMDGCNSPIRV (where the hardware targets are known). We also set the stack alignment on the latter, as it is overaligned (32-bit vs 8-bit).
Add `using namespace Intrinsic` to Intrinsics.cpp file.
…lvm#100142) The `RegisterBuiltinMacro` function has been moved to the Preprocessor class for accessibility and has been refactored for readability.
…llvm#114924) For a VOP3 instruction that does not permit a literal operand with an SGPR operand, this would re-use the same scratch register for both operands, clobbering the original value.
This reverts commit 25909b8 due to unresolved questions about the behavior of "frame var" and ValueObject in the presence of references (see the original patch for discussion).
Add new builders to DataBoundsOp, data entry ops, and data exit ops to simplify their construction since many of their inputs are optional. Additionally, small refactoring was needed for data exit ops to reduce duplication. Unit tests were added to exercise the new builders.
NF7 and NF8 were just missing from the coverage. The one active lane cases should be a strided load instead.
…face (llvm#114390) ACCMP atomics do not support type conversion. Specifically, I have encountered semantically incorrect code for atomic reads. Example: ``` program main implicit none real(8) :: n integer :: x x = 1.0 !$acc atomic capture n = x x = n !$acc end atomic end program main ``` We have this error when compiling it with flang-new: `error: loc("rep.f90":6:9): expected three operations in atomic.capture region (one terminator, and two atomic ops)` Yet, in the following generated FIR code, we observe three issues. 1. `fir.convert` intrudes into the capture region. 2. An incorrect temporary (`%2`) is being updated instead of `n`. 3. If we allow `n` in place of `%2`, the operand types of `atomic.read` do not match. Introducing a `!fir.ref<i32> -> !fir.ref<f64>` conversion on `x` is inaccurate because we need to convert the value of `x`. ``` %2 = "fir.alloca"() <{in_type = i32, operandSegmentSizes = array<i32: 0, 0>}> : () -> !fir.ref<i32> %3 = "fir.alloca"() <{bindc_name = "n", in_type = f64, operandSegmentSizes = array<i32: 0, 0>, uniq_name = "_QFEn"}> : () -> !fir.ref<f64> %4:2 = "hlfir.declare"(%3) <{operandSegmentSizes = array<i32: 1, 0, 0, 0>, uniq_name = "_QFEn"}> : (!fir.ref<f64>) -> (!fir.ref<f64>, !fir.ref<f64>) %5 = "fir.alloca"() <{bindc_name = "x", in_type = i32, operandSegmentSizes = array<i32: 0, 0>, uniq_name = "_QFEx"}> : () -> !fir.ref<i32> %6:2 = "hlfir.declare"(%5) <{operandSegmentSizes = array<i32: 1, 0, 0, 0>, uniq_name = "_QFEx"}> : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>) %7 = "arith.constant"() <{value = 1 : i32}> : () -> i32 "hlfir.assign"(%7, %6#0) : (i32, !fir.ref<i32>) -> () %8 = "fir.load"(%4#0) : (!fir.ref<f64>) -> f64 %9 = "fir.convert"(%8) : (f64) -> i32 "fir.store"(%9, %2) : (i32, !fir.ref<i32>) -> () %10 = "fir.load"(%6#0) : (!fir.ref<i32>) -> i32 %11 = "fir.convert"(%10) : (i32) -> f64 "acc.atomic.capture"() ({ "acc.atomic.read"(%2, %6#1) <{element_type = f64}> : (!fir.ref<i32>, !fir.ref<i32>) -> () %12 = "fir.convert"(%11) : (f64) -> i32 "acc.atomic.write"(%2, %12) : (!fir.ref<i32>, i32) -> () "acc.terminator"() : () -> () }) : () -> () ``` This PR updates `flang/lib/Lower/DirectivesCommon.h` to solve the issues by taking the following approaches (from top to bottom): 1. Move `fir.convert` for `atomic.write` out of the capture region. 2. Remove the `!fir.ref<i32> -> !fir.ref<f64>` conversion found in `genOmpAccAtomicRead`. 3. Eliminate unnecessary `genExprAddr` calls on the RHS, which create an invalid temporary for `x = 1.0`. 4. When generating a capture operation, refer to the original LHS instead of the type-casted RHS. Here, we have to allow for the cases where the operand types of `atomic.read` differ from one another. Thus, this PR also removes the `AllTypesMatch` trait from both `acc.atomic.read` and `omp.atomic.read`. The example code is converted as follows: ``` %0 = fir.alloca f64 {bindc_name = "n", uniq_name = "_QFEn"} %1:2 = hlfir.declare %0 {uniq_name = "_QFEn"} : (!fir.ref<f64>) -> (!fir.ref<f64>, !fir.ref<f64>) %2 = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFEx"} %3:2 = hlfir.declare %2 {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>) %c1_i32 = arith.constant 1 : i32 hlfir.assign %c1_i32 to %3#0 : i32, !fir.ref<i32> %4 = fir.load %1#0 : !fir.ref<f64> %5 = fir.convert %4 : (f64) -> i32 acc.atomic.capture { acc.atomic.read %1#1 = %3#1 : !fir.ref<f64>, !fir.ref<i32>, i32 acc.atomic.write %3#1 = %5 : !fir.ref<i32>, i32 } ``` Fixes llvm#112911.
Chris has extensive knowledge of HLSL and a long track-record of quality reviews in Clang.
Mariya has extensive knowledge of the constant expression evaluator and has done a lot of reviewing in Sema over the past year or so.
The only way to test `isProjectedPermutation` is through unit tests. The concept of "projected permutations" is tricky to document and these tests are a good source documentation of the expected/intended behavoiur. Hence these additional unit tests.
The `sycl_kernel_entry_point` attribute is used to declare a function that defines a pattern for an offload kernel to be emitted. The attribute requires a single type argument that specifies the type used as a SYCL kernel name as described in section 5.2, "Naming of kernels", of the SYCL 2020 specification. Properties of the offload kernel are collected when a function declared with the `sycl_kernel_entry_point` attribute is parsed or instantiated. These properties, such as the kernel name type, are stored in the AST context where they are (or will be) used for diagnostic purposes and to facilitate reflection to a SYCL run-time library. These properties are not serialized with the AST but are recreated upon deserialization. The `sycl_kernel_entry_point` attribute is intended to replace the existing `sycl_kernel` attribute which is intended to be deprecated in a future change and removed following an appropriate deprecation period. The new attribute differs in that it is enabled for both SYCL host and device compilation, may be used with non-template functions, explicitly indicates the type used as the kernel name type, and will impact AST generation. This change adds the basic infrastructure for the new attribute. Future changes will add diagnostics and new AST support that will be used to drive generation of the corresponding offload kernel.
Broken first by llvm#114620
…r > 1 Use (llvm#114077) This PR fixes the `ThrowingT` class, which currently fails to raise exceptions after a specified number of copy construction operations. The class is intended to throw in a controlled manner based on a specified counter value `throw_after`. However, its current implementation of the copy constructor fails to achieve this goal. The problem arises because the copy constructor does not initialize the `throw_after_n_` member, leaving `throw_after_n_` to default to `nullptr` as defined by the in-class initializer. As a result, its copy constructor always checks against `nullptr`, causing an immediate exception rather than throwing after the specified number `throw_after` of uses. The fix is straightforward: simply initialize the `throw_after_n_` member in the member initializer list. This issue was previously uncovered because all exception tests for `std::vector` in `exceptions.pass.cpp` used a `throw_after` value of 1, which coincidentally aligned with the class's behavior.
Saleem has upstreamed a large chunk of API Notes infrastructure from the Apple fork, and over the past year I've upstreamed the remaining part of API Notes, added new annotations and improved C++ language support. https://github.com/llvm/llvm-project/commits/main/clang/lib/APINotes
This is still a draft since other tests fail and the Objective-C tests also still need to be implemented
Contributor
Author
|
I'm sorry to have notified you all. Instead of rebasing I merged upstream changes which resulted in all these commits being included in the PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an unfinished implementation of #33528. I have a couple of questions since I'm new to the codebase:
This warning should be issued for both C and C++. For C++ it is pretty straightforward since I can check this when calling
PerformImplicitConversion. However this function isn't called in C and right now I issue the warning for C inSema::ImpCastExprToType, but I don't know if this function might also be called in C++ resulting in the same warning issued twice.Ideally I would just have to issue this once for both languages but I can't seem to find the correct
ImplicitCastfunction that gets invoked for both langs.I also found another already existing warning when running the tests (warn_condition_is_assignment) that is similar to this warning but only gets issued on assignments in conditional-expressions, as opposed to all assignment expressions. In C++ this results in the new and old warning both being displayed even though they both suggest the same thing.
How should this be handled? Should I manually check if the broader warning has already been issued and then remove it or maybe just keep the more general warning as they mean the same thing (-Wparentheses)?
Any feedback is appreciated!