Skip to content

Conversation

@PhilippRados
Copy link
Contributor

@PhilippRados PhilippRados commented Nov 2, 2024

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 in Sema::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 ImplicitCast function 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!

@github-actions
Copy link

github-actions bot commented Nov 2, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

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.cpp
View 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.

@Endilll Endilll added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Nov 5, 2024
kazutakahirata and others added 28 commits November 6, 2024 11:01
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.
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.
…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.
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.
preames and others added 24 commits November 6, 2024 11:02
…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.
…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
@PhilippRados PhilippRados deleted the implicit_cast_bool_assign branch November 6, 2024 10:08
@PhilippRados
Copy link
Contributor Author

PhilippRados commented Nov 6, 2024

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

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema"

Projects

None yet

Development

Successfully merging this pull request may close these issues.