[CIR][Dialect][NFC] Add some helpers to LoadOp #1021

lanza · 2024-10-29T17:52:59Z

These are just missing getters/setters that should be there already.
They are in use in a patch coming up. I'm splitting them out here for
reviewability.

This PR adds CIRGen and LLVMIR codegen for those not-yet-covered complex casts, including explicit type cast expressions of complex types and complex value promotion. All type conversion expressions involving complex types should now codegen.

This PR verifies `cir.get_global` has its result type correctly annotated with address space of the referenced symbol. The documentation is also updated to clarify this constraint. `GlobalOp` is the main consideration. It's worth noting that if the `cir.get_global` op references a function, we also (implicitly) checks that its result pointer type has no address space attribute.

This PR sets proper address space when creating `cir.global` and `cir.get_global`. Different languages use different ways to encode the address space in AST constructs (i.e. VarDecl *). OpenCL and SYCL use an address space qualifier on the type of `VarDecl`, while CUDA uses separate AST attributes like `CUDASharedAttr`. Similarily, some targets may want to use special address space for global variables. So a per-language + per-target hook is needed to provide this customization point. In the LLVM CodeGen, it's the helper method `getGlobalVarAddressSpace` that takes on the role. For OpenCL C + SPIR-V combination, OpenCL C converts the address space qualifier to corresponding LangAS, but SPIR-V does not require any action. This PR implements `global` qualifier in OpenCL C, but does not include `constant` qualifier. Although the modified part works for `constant`, CIRGen is not yet able to set constant attribute for global ops (there is a TODO line). Static variable decl and `local` qualifier work in a similar way and come in later patches.

Unary decrement expression on floating point operands was lowered to `fsub -1.0` by a typo. This PR fixes this bug.

…otected (#776) This PR add a new CIR attribute `mlir::cir::VisibilityAttr` to represent CIR visibility. It will represent C/C++ visibility type `Default`, `Hidden`, `Protected`. The PR handles the parsing, printing of CIR visibility and also lower to LLVM. After this PR, there will be more PR's to migrate CIRGen properties that are currently querying MLIR visibility(e.g. `sym_visibility`), to instead query CIR visibility, and remove MLIR's visibility from printing and parsing.

This PR adds CIRGen and LLVMIR lowering for unary increment and decrement expressions of complex types. Currently blocked by #789 .

…d variables (#792) In OpenCL, `local`-qualified variables are implicitly considered as static. In order to support it, this PR unblocks code paths related to OpenCL static declarations in `CIRGenDecl.cpp`. Following the approach of LLVM CodeGen, a new class `CIRGenOpenCLRuntime` is added to handle the language hook of creating `local`-qualified variables. The default behavior of this hook is quite simple. It forwards the call to `CGF.buildStaticVarDecl`. So in CIR, the OpenCL local memory representation is equivalent to the one defined by SPIR-LLVM convention: a `cir.global` with `addrspace(local)` and *without initializer*, which corresponds to LLVM `undef` initializer. See check lines in test for more details. A `static global`-qualified variable is also added in the test to exercise the static code path itself.

This patch fixes a bunch of pending review comments in #784: - Remove data layout attribute from address space testing - Remove incoherent comment - Rename abi_or_pref to abiOrPref - Make comments impersonal - Implement feature guard for ARM's CMSE secure call feature - Track volatile return times feature in CC lowering - Track missing features in the Itanium record builder - Remove incoherent fix me - Clarify comment regarding CIR record layout getter - Track missing cache for record layout getter - Remove unnecessary todo's

This PR adds the initial CIRGen support for pointer-to-member-functions. It contains the following new types, attributes, and operations: - `!cir.method`, which represents the pointer-to-member-function type. - `#cir.method`, which represents a literal pointer-to-member-function value that points to ~~non-virtual~~ member functions. - ~~`#cir.virtual_method`, which represents a literal pointer-to-member-function value that points to virtual member functions.~~ - ~~`cir.get_method_callee`~~ `cir.get_method`, which resolves a pointer-to-member-function to a function pointer as the callee. See the new test at `clang/test/CIR/CIRGen/pointer-to-member-func.cpp` for how these new CIR stuff works to support pointer-to-member-functions.

Achieve union's bitfields additionally.

This PR adds a new `cir.select` operation. This operation won't be generated directly by CIRGen but it is useful during further CIR to CIR transformations. This PR addresses #785 .

There is an implementation for inline variables processing at CIR. The LIT test was taken from clang's cxx1z-inline-variables.cpp where the same functionality is tested for Clang Code generation. The test can be run as follows ``` bin/llvm-lit -v ../clang/test/CIR/CodeGen/cxx1z-inline-variables.cpp ``` Note: the pull request also contains a formatting change for two files: - `clang/lib/CIR/Dialect/IR/CIRDataLayout.cpp` - `clang/lib/CIR/Dialect/Transforms/TargetLowering/Targets/X86.cpp`

mixed signed and unsigned integer operator cause difference result when index of array is negative

…800) This PR refactors the LoweringPrepare pass and replaces various ternary ops generated by LoweringPrepare with semantically equivalent select ops.

#799) `__sync_fetch_and_add` currently doesn't support unsigned integers. The following code snippet, for example, raises an error: ``` #include <stdint.h> void foo(uint64_t x) { __sync_fetch_and_add(&x, 1); } ``` The error can be traced down to this line `auto intType = builder.getSIntNTy(cgf.getContext().getTypeSize(typ));` from `clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp`.

@philnik777

…ry fp2fp operations (#806) This PR is the continuation and refinement of PR #434 which is originally authored by @philnik777 . Does not update it in-place since I don't have commit access to Nikolas' repo. This PR basically just rebases #434 onto the latest `main`. I also updated some naming used in the original PR to keep naming styles consistent. Co-authored-by: Nikolas Klauser <nikolasklauser@berlin.de>

…r try local allocas

Still missing CFG flattening and lowering, coming next.

…pushTemporaryCleanup

We had some incorrect logic when creating functions and getting their address which resulted in spurious "definition with the same mangled name" errors. Fix that logic to match original CodeGen, which also fixes these errors. It's expected that decls can appear in the deferred decl list multiple times, and CodeGen has to guard against that. In the case that triggered the error, both `CIRGenerator::HandleInlineFunctionDefinition` and CIRGenModule were deferring the declaration. Something else I discovered here is that we emit these functions in the opposite order as regular CodeGen: https://godbolt.org/z/4PrKG7h9b. That might be a meaningful difference worth investigating further. Fixes #991

…intrinsics (#1020) In this PR, also changed `buildNeonShiftVector` to allow it generates negative shift values. When the shift value is negative, the shift amount vector is not used in any ShiftOp of IR (as they don't need sign to know shift direction), instead, it is just input argument to shift intrinsic function call.

bcardosolopes

We already have enough assert checks for syncScopeID in clang/lib/CIR/CodeGen/CIRGenAtomic.cpp, please remove setAtomic and use setMemOrder directly. Feel free to add more asserts in CIRGen if you need, but no need to touch the operation this time around.

lanza · 2024-10-30T19:19:54Z

We already have enough assert checks for syncScopeID in clang/lib/CIR/CodeGen/CIRGenAtomic.cpp, please remove setAtomic and use setMemOrder directly. Feel free to add more asserts in CIRGen if you need, but no need to touch the operation this time around.

That's not right. Mapping setAtomic to setMemOrder is incorrect in cases where a syncScopeID should be there. All callers of setAtomic that should have a syncScopeID should crash as soon as we support syncScopeID to ensure they adjust their arguments. Threading use cases to setMemOrder hides this.

Mapping

SyncScopeID ssid = getSyncScope();
AtomicOrdering ao = getAtomicOrdering();
m.setAtomic(ao, ssid);

to

// TODO(CIR): syncscopeid
AtomicOrdering ao = getAtomicOrdering();
m.setMemOrder(ao);

won't be fixed until somebody is diligent about fixing all our TODOs. Assertion failures require fixing and will catch it immediately.

) This PR fixes the notorious double whitespaces introduced by visibility attribute, for `cir.func` only. It uses "leading whitespace" for every print. And the printing of visibility attr is properly guarded by a check of `!isDefault()`. Double whitespaces in test files are removed.

bcardosolopes · 2024-10-30T19:40:57Z

Well, the way this PR handles it doesn't help much either: setMemOrder can still be used and we have to catch that at review time, which is unreliable in the long run (e.g. reviewers without full context).

If that's the route you want to take, then please also do the following extra steps:

Change all calls to setMemOrder to setAtomic.
Add the same logic for StoreOp.
Add a comment to setAtomic saying folks should prefer to use that then setMemOrder directly.

This patch introduces support for the abs family of built-in functions (abs, labs, llabs).

Add lowering prepare logic to lower stores to cir.copy. This bring LLVM lowering closer to OG and turns out the rest of the compiler understands memcpys better and generate better assembly code for at least arm64 and x86_64. Note that current lowering to memcpy is only using i32 intrinsic version, this PR does not touch that code and that will be addressed in another PR.

POSION -> POISON

Clang recognizes the function anyway, but this is an obvious error.

In addition, this PR enables ZeroAttr of vector type so that CIR can generate a vector initialized with all zero values.

due to the issue described in #1018, the MLIR lowering for `memmove` has been excluded in this patch.

…bits (#1027) This PR adds a support for the return values of struct types > 128 bits in size. As usually, lot's of copy-pasted lines from the original codegen, except the `AllocaOp` replacement for the return value.

keryell · 2024-10-31T21:17:23Z

clang/lib/CIR/Dialect/IR/CIRDialect.cpp

@@ -49,6 +49,7 @@ using namespace mlir;
 #include "clang/CIR/Dialect/IR/CIROpsDialect.cpp.inc"
 #include "clang/CIR/Interfaces/ASTAttrInterfaces.h"
 #include "clang/CIR/Interfaces/CIROpInterfaces.h"
+#include <clang/CIR/MissingFeatures.h>


Curious this different inclusion style.
Anyway, I am compassionate about this blast effect 21f9795

Yea, this looks off, unless everything changes to angle based, this should be quotes based in this PR.

What exactly do you mean? I'm including MissingFeatuers here because it's the right thing to do to ensure we're guarding against the SSID. But I agree that it's really ugly. Hopefully we see some change from #1025!

We mean just a minor

Suggested change

#include <clang/CIR/MissingFeatures.h>

#include "clang/CIR/MissingFeatures.h"

Good to go after you fix this one!

@sitio-couto

… convention lowering pass (#1034) This PR adds a support for calls by function pointers. @sitio-couto I think would be great if you'll also take a look

…in_operator_delete (#1035) The added test cases are from [OG's counterpart](https://github.com/llvm/clangir/blob/f9c5477ee10c9bc005ffbfe698691cc02193ea81/clang/test/CodeGenCXX/builtin-operator-new-delete.cpp#L7). I changed run option to -std=c++17 to support [std::align_val_t](https://en.cppreference.com/w/cpp/memory/new/align_val_t)

This PR adds calling convention lowering support for the int128 type on x86_64. This is a follow up on #953 .

…1037)

Created using spr 1.3.5

done

These are just missing getters/setters that should be there already. They are in use in a patch coming up. I'm splitting them out here for reviewability. Pull Request: #1021

Created using spr 1.3.5

These are just missing getters/setters that should be there already. They are in use in a patch coming up. I'm splitting them out here for reviewability. Reviewers: bcardosolopes Pull Request: #1021

Lancern and others added 30 commits October 12, 2024 00:29

[CIR] Fix wrong LLVMIR lowering of fp decrement (#789)

1e9382e

Unary decrement expression on floating point operands was lowered to `fsub -1.0` by a typo. This PR fixes this bug.

[CIR][CIRGen] Complex unary increment and decrement operator (#790)

4eb034b

This PR adds CIRGen and LLVMIR lowering for unary increment and decrement expressions of complex types. Currently blocked by #789 .

[CIR][CIRGen] Achieve union's bitfields additionally. (#742)

970efe9

Achieve union's bitfields additionally.

[CIR] Cleanup most recent warnings

2f82a2a

[CIR] More warning fixed by another fresh build

e6ac8e5

[CIR] Add select operation (#796)

253d1e3

This PR adds a new `cir.select` operation. This operation won't be generated directly by CIRGen but it is useful during further CIR to CIR transformations. This PR addresses #785 .

[CIR] Incorrect global view index and offset when neg index (#795)

8dc84ff

mixed signed and unsigned integer operator cause difference result when index of array is negative

[CIR][NFC] Replace ternary ops after lowering prepare to select ops (#…

2f86e11

…800) This PR refactors the LoweringPrepare pass and replaces various ternary ops generated by LoweringPrepare with semantically equivalent select ops.

[CIR][CIRGen] Support global initialization with new

d784759

[CIR][NFC] Fix sign-compare warning

f13d2ec

[CIR] Add missing testcase for previous commit

49ceda3

[CIR][CIRGen] Add minimal support for building invariant globals

307fd34

[CIR][CIRGen] Exceptions: Use the surrounding scope (if available) fo…

ec31a4a

…r try local allocas

[CIR][NFC] Exceptions: introduce cleanup region to cir.try

e9527a6

[CIR][CIRGen] Exceptions: handle cleanups and global initializers

806b457

Still missing CFG flattening and lowering, coming next.

[CIR][FlattenCFG] Exceptions: propagate cleanups to flat CIR

14dd633

[CIR] Add new HLSLResource enum value coverage

c0c66d7

[CIR] Exceptions: propagate more cleanup info for LLVM lowering use

1529f0e

[CIR][CIRGen][NFC] Add more skeleton to make crashes fine grained on …

8ef6fce

…pushTemporaryCleanup

[CIR][NFC] Exceptions: Move a crash to an assert

672ade9

ghehg and others added 4 commits October 30, 2024 11:27

[CIR][CIRGen][Builtin][Neon] Lower neon_vqmovun_v (#1012)

d6db31d

[CIR][CIRGen] Enable comdat for static variables (#1015)

29e5998

bcardosolopes previously requested changes Oct 30, 2024

View reviewed changes

PikachuHyA and others added 7 commits October 30, 2024 12:41

[CIR][CIRGen] Add support for abs (#1011)

5404e9c

This patch introduces support for the abs family of built-in functions (abs, labs, llabs).

[CIR][CIRGen] Fix typo in test check

f9c5477

POSION -> POISON

[CIR][CIRGen] Fix swapped parameters in test

b53784a

Clang recognizes the function anyway, but this is an obvious error.

[CIR][CIRGen][Builtin][Neon] Lower neon_vtst_v and neon_vtstq_v (#1013)

7e7be0e

In addition, this PR enables ZeroAttr of vector type so that CIR can generate a vector initialized with all zero values.

[CIR][CIRGen] Add support for memmove (#1019)

2813bac

due to the issue described in #1018, the MLIR lowering for `memmove` has been excluded in this patch.

keryell reviewed Oct 31, 2024

View reviewed changes

gitoleg and others added 6 commits October 31, 2024 14:21

[CIR][ABI][Lowering] Supports call by function pointer in the calling…

ce2469e

… convention lowering pass (#1034) This PR adds a support for calls by function pointers. @sitio-couto I think would be great if you'll also take a look

[CIR][ABI][Lowering] Add CCLower support for int128 on x86_64 (#1036)

734d344

This PR adds calling convention lowering support for the int128 type on x86_64. This is a follow up on #953 .

[CIR][CIRGen][Builtin][Neon] Lower neon_vqshlu_n and neon_vqshluq_n (#…

dae8726

…1037)

[CIR][CIRGen] Support more member init variations

3d16a0f

storeop and usages

37c7e20

Created using spr 1.3.5

lanza requested a review from bcardosolopes November 1, 2024 18:41

lanza force-pushed the main branch from 3ef67c1 to adf0c99 Compare November 4, 2024 00:41

rebase

67264e8

Created using spr 1.3.5

lanza merged commit 9dfe49d into main Nov 5, 2024
53 of 55 checks passed

lanza deleted the spr/lanza/cirdialect-add-some-helpers-to-loadop branch November 5, 2024 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIR][Dialect][NFC] Add some helpers to LoadOp #1021

[CIR][Dialect][NFC] Add some helpers to LoadOp #1021

lanza commented Oct 29, 2024

bcardosolopes left a comment

lanza commented Oct 30, 2024 •

edited

Loading

bcardosolopes commented Oct 30, 2024

keryell Oct 31, 2024

bcardosolopes Oct 31, 2024

lanza Nov 1, 2024

keryell Nov 1, 2024

bcardosolopes Nov 2, 2024

	#include <clang/CIR/MissingFeatures.h>
	#include "clang/CIR/MissingFeatures.h"

[CIR][Dialect][NFC] Add some helpers to LoadOp #1021

[CIR][Dialect][NFC] Add some helpers to LoadOp #1021

Conversation

lanza commented Oct 29, 2024

bcardosolopes left a comment

Choose a reason for hiding this comment

lanza commented Oct 30, 2024 • edited Loading

bcardosolopes commented Oct 30, 2024

keryell Oct 31, 2024

Choose a reason for hiding this comment

bcardosolopes Oct 31, 2024

Choose a reason for hiding this comment

lanza Nov 1, 2024

Choose a reason for hiding this comment

keryell Nov 1, 2024

Choose a reason for hiding this comment

bcardosolopes Nov 2, 2024

Choose a reason for hiding this comment

lanza commented Oct 30, 2024 •

edited

Loading