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

[CodeGen] Migrate away from PointerUnion::dyn_cast (NFC) #122653

Conversation

kazutakahirata
Copy link
Contributor

Note that PointerUnion::dyn_cast has been soft deprecated in
PointerUnion.h:

// FIXME: Replace the uses of is(), get() and dyn_cast() with
// isa, cast and the llvm::dyn_cast

Literal migration would result in dyn_cast_if_present (see the
definition of PointerUnion::dyn_cast), but this patch uses dyn_cast
because we expect Prototype.P to be nonnull.

Note that PointerUnion::dyn_cast has been soft deprecated in
PointerUnion.h:

  // FIXME: Replace the uses of is(), get() and dyn_cast() with
  //        isa<T>, cast<T> and the llvm::dyn_cast<T>

Literal migration would result in dyn_cast_if_present (see the
definition of PointerUnion::dyn_cast), but this patch uses dyn_cast
because we expect Prototype.P to be nonnull.
@kazutakahirata kazutakahirata requested a review from nikic January 12, 2025 23:17
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen labels Jan 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 12, 2025

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Kazu Hirata (kazutakahirata)

Changes

Note that PointerUnion::dyn_cast has been soft deprecated in
PointerUnion.h:

// FIXME: Replace the uses of is(), get() and dyn_cast() with
// isa<T>, cast<T> and the llvm::dyn_cast<T>

Literal migration would result in dyn_cast_if_present (see the
definition of PointerUnion::dyn_cast), but this patch uses dyn_cast
because we expect Prototype.P to be nonnull.


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

1 Files Affected:

  • (modified) clang/lib/CodeGen/CGCall.cpp (+1-1)
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 0fde4d8ee296bd..a71af0141709fd 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -4507,7 +4507,7 @@ void CodeGenFunction::EmitCallArgs(
   // First, if a prototype was provided, use those argument types.
   bool IsVariadic = false;
   if (Prototype.P) {
-    const auto *MD = Prototype.P.dyn_cast<const ObjCMethodDecl *>();
+    const auto *MD = dyn_cast_if_present<const ObjCMethodDecl *>(Prototype.P);
     if (MD) {
       IsVariadic = MD->isVariadic();
       ExplicitCC = getCallingConventionForDecl(

@nikic
Copy link
Contributor

nikic commented Jan 13, 2025

Literal migration would result in dyn_cast_if_present (see the definition of PointerUnion::dyn_cast), but this patch uses dyn_cast because we expect Prototype.P to be nonnull.

This comment doesn't seem to match the PR :)

bogner and others added 24 commits January 13, 2025 11:44
This introduces `@llvm.dx.resource.store.rawbuffer` and generalizes the
buffer store docs under DirectX/DXILResources.

Fixes llvm#106188
This commit fixes -print-library-module-manifest-path on macos.
Currently, this only works on linux systems. This is because on macos
systems the library and header files are installed in a different
location. The module manifest is next to the libraries and the search
function was not looking in both places. There is also a test included.
Use descriptive names and add more cases.
Reverts llvm#120364

The test should be updated due to some recent changes.
Use descriptive names and add more cases.

This recommits 59bba39 which was reverted in 4637c77.
- Implementation of tan for 16-bit floating point inputs.
- Exhaustive tests across the 16-bit input range
`PassRegistry.def` already has this entry, but the dummy definition was
being pulled instead.

I couldn't reproduce the build failures that FIXME referenced, maybe the
Dummy pass getting in the way was part of the cause.
Before this patch, [godbolt](https://godbolt.org/z/6PPsvv9qd) failed to
compile `cfloat128` with `-ffreestanding` but with the patch, the
compilation succeeds, [godbolt](https://godbolt.org/z/4M8zzejss).

Fixes: llvm#122500

cc: @nickdesaulniers
llvm#121914)

Added codegen support for combined masked constructs `masked taskloop.`
Added implementation for `EmitOMPMaskedTaskLoopDirective`.

---------

Co-authored-by: Chandra Ghale <[email protected]>
The decomposition of `linalg.softmax` uses `maxnumf`, but the identity
element that is used in the generated code is the one for `maximumf`.
They are not the same, as the identity for `maxnumf` is `NaN`, while the
one of `maximumf` is `-Infty`. This is wrong and prevents the maxnumf
from being folded.

Related to llvm#114595, which fixed the folder for maxnumf.
…122586)"

This partially reverts commit 07ff786.

The hunk being reverted in this patch seems to break:

  tools/llvm-gsymutil/ARM_AArch64/macho-merged-funcs-dwarf.yaml

under LLVM_ENABLE_EXPENSIVE_CHECKS.
…1549)

This patch simplifies select-based integer min/max reductions by
utilizing `llvm::getMinMaxReductionPredicate`, and generates
intrinsic-based min/max reductions by utilizing
`llvm::getMinMaxReductionIntrinsicOp`.
…m#119181)" (llvm#122665)

Makes Inline Spiller amenable to the new PM.

This reapplies commit a531800 reverted
because of two unused private members reported on sanitizer bots.
…118466)

`ASTImporterLookupTable` did use the `getPrimaryContext` function to get
the declaration context of the inserted items. This is problematic
because the primary context can change during import of AST items, most
likely if a definition of a previously not defined class is imported.
(For any record the primary context is the definition if there is one.)
The use of primary context is really not important, only for namespaces
because these can be re-opened and lookup in one namespace block is not
enough. This special search is now moved into ASTImporter instead of
relying on the lookup table.
…till a failure (llvm#120264)

In this build:
https://buildkite.com/llvm-project/github-pull-requests/builds/126961

The builds actually failed, probably because prerequisite of a test
suite failed to build.

However they still ran other tests and all those passed. This meant that
the test reports were green even though the build was red. On some level
this is technically correct, but it is very misleading in practice.

So I've also passed the build script's return code, as it was when we
entered the on exit handler, to the generator, so that when this happens
again, the report will draw the viewer's attention to the overall
failure. There will be a link in the report to the build's log file, so
the next step to investigate is clear.

It would be nice to say "tests failed and there was some other build
error", but we cannot tell what the non-zero return code was caused by.
Could be either.

The script handles the following situations now:
| Have Result Files? | Tests reported failed? | Return code | Report |

|--------------------|------------------------|-------------|-----------------------------------------------------------------------------|
| Yes | No | 0 | Success style report. |
| Yes | Yes | 0 | Shouldn't happen, but if it did, failure style report
showing the failures. |
| Yes | No | 1 | Failure style report, showing no failures but noting
that the build failed. |
| Yes | Yes | 1 | Failure style report, showing the test failures. |
| No | ? | 0 | No test report, success shown in the normal build
display. |
| No | ? | 1 | No test report, failure shown in the normal build
display. |
…d is bf16 (llvm#122664)

The PR fixes the datatype error for `nvvm.mma.sync` when the operand is
`bf16`. This operation originally requires the A/B type to be `f16x2`
for the `bf16` MMA. However, it violates the NVVM intrinsic
[[here](https://github.com/xiaoleis-nv/llvm-project/blob/372044ee09d39942925824f8f335aef40bfe92f0/llvm/include/llvm/IR/IntrinsicsNVVM.td#L119)],
where the A/B operand type should be `i32`. This is a bug, and there are
no tests in MLIR that cover this datatype.

```
    // mma bf16 -> s32 @ m16n8k16/m16n8k8
    !eq(gft,"m16n8k16:a:bf16") : !listsplat(llvm_i32_ty, 4),
    !eq(gft,"m16n8k16:b:bf16") : !listsplat(llvm_i32_ty, 2),
    !eq(gft,"m16n8k8:a:bf16") : !listsplat(llvm_i32_ty, 2),
    !eq(gft,"m16n8k8:b:bf16") : [llvm_i32_ty],
```

This PR addresses this bug and adds tests to guarantee correctness.

Co-authored-by: Xiaolei Shi <[email protected]>
…22472)

When passing an instruction with a register mask, the machine copy
propagation pass was dropping the information about some copy
instructions which define a register which is preserved by the mask,
because that register overlaps a register which is partially clobbered
by it. This resulted in a miscompilation for AArch64, because this
caused a live copy to be considered dead.

The fix is to clobber register masks by finding the set of reg units
which is preserved by the mask, and clobbering all units not in that
set.
…vm#122462)

Add a release note for optimization change related to pointer overflow
checks. I've put this in the breaking changes section to give it the
best chance of being seen.
…llvm#122675)

The test was effectively a no-op since we used `//` instead of `!` for
`RUN` and `CHECK` lines. Also, we have to specify the proper OpenMP
version.
…vm#122303)

Do not use the PrintFatalError diagnostic machinery for conditions that
can never happen with any input.
andjo403 and others added 16 commits January 13, 2025 11:44
The NSError summary provider was fetching and printing the `_code` field
as an unsigned integer, but it's defined to be an NSInteger, which is
signed.
…122157)

- Adding the changes from PRs: 
  - llvm#116331 
  - llvm#121852 
- Fixes test `tools/dxil-dis/debug-info.ll`
- Address some missed comments in the previous PR

---------

Co-authored-by: joaosaffran <[email protected]>
…tter pointer size in Intel syntax. (llvm#122530)

This matches binutils.
When extracts are vectorized and it has some poison values instead of
instructions, need to correctly set the vectorized operand not as
poison, but as a main vector operand of the main extract instruction.

Fixes llvm#122583
The header <features.h> is a system header. It's not part of the headers
in __cxx03.
PointerUnion::{is,get} have been soft deprecated in PointerUnion.h:

  // FIXME: Replace the uses of is(), get() and dyn_cast() with
  //        isa<T>, cast<T> and the llvm::dyn_cast<T>

This patch actually deprecates them with [[deprecated]].

I'm not touching PointerUnion::dyn_cast for now because we have not
migrated away from it yet.
Note that PointerUnion::dyn_cast has been soft deprecated in
PointerUnion.h:

  // FIXME: Replace the uses of is(), get() and dyn_cast() with
  //        isa<T>, cast<T> and the llvm::dyn_cast<T>

Literal migration would result in dyn_cast_if_present (see the
definition of PointerUnion::dyn_cast), but this patch uses dyn_cast
because we expect Ptr to be nonnull.
Note that PointerUnion::dyn_cast has been soft deprecated in
PointerUnion.h:

  // FIXME: Replace the uses of is(), get() and dyn_cast() with
  //        isa<T>, cast<T> and the llvm::dyn_cast<T>

Literal migration would result in dyn_cast_if_present (see the
definition of PointerUnion::dyn_cast), but this patch uses dyn_cast
because we expect Ctx->FunArgs to be nonnull.
@kazutakahirata
Copy link
Contributor Author

Oops. Let me delete this PR and start over. Sorry about the mess.

@kazutakahirata kazutakahirata deleted the cleanup_001_PointerUnion_clang_CodeGen branch January 13, 2025 19:49
@ldionne ldionne removed the request for review from a team January 14, 2025 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.