[CIR][CIRGen][TBAA] Add support for TBAA #1076
Open
Conversation
This file contains 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
…pushTemporaryCleanup
- Tackle a FIXME left in previous commit. - Start turning missing cleanup features into actual NYI asserts. - Make sure we run CleanupDeactivationScope properly and deactivate necessary cleanups. - Add more verifications/asserts for overall state.
This is currently crashing on Linux only internally, but not on GitHub's CI, disable it temporarily while we investigate.
This is a straightforward adaption of existing CodeGen logic. While I'm here, move block comments inside their blocks, so that they look nicer.
…VM (llvm#810) As title. Add setAlignmentAttr for GlobalOps created from AST. LLVM Lowering should have LLVM GlobalOp's alignment attribute inherited from CIR::GlobalOp. This PR is definitely needed to fix issue llvm#801 (comment), but the issue doesn't have alignment and comdat attribute for CIR Ops to begin with, so I'll keep investigating and fix CIR problem in another PR.
…y cast op (llvm#812) There are two occurrences of `cir.cast(array_to_ptrdecay, ...)` that drop address spaces unexpectedly for its result pointer type. This PR fixes them with the source address space. ```mlir // Before %1 = cir.cast(array_to_ptrdecay, %0 : !cir.ptr<!cir.array<!s32i x 32>, addrspace(offload_local)>), !cir.ptr<!s32i> // After %1 = cir.cast(array_to_ptrdecay, %0 : !cir.ptr<!cir.array<!s32i x 32>, addrspace(offload_local)>), !cir.ptr<!s32i, addrspace(offload_local)> ```
As mentioned at llvm#809 (comment) , this PR adds more simplify transformations for select op: - `cir.select if %0 then x else x` -> `x` - `cir.select if %0 then #true else #false` -> `%0` - `cir.select if %0 then #false else #true` -> `cir.unary not %0`
…p ops (llvm#818) This PR makes simple lowering generate the result type lowering logic and make it suitable for unary fp2int operations and binary fp2fp operations.
…ization (llvm#820) This PR adds initial support for temporary materialization of local temporaries with trivial cleanups. Materialization of global temporaries and local temporaries with non-trivial cleanups is far more trickier that I initially thought and I decide to submit this easy part first.
This is a straightforward adaption from CodeGen. I checked the uses of the Delegating arg that's passed in various places, and it only appears to be used by virtual inheritance, which should be handled by llvm#624.
…on (llvm#813) Currently some bitcasts would silently change the address space of source pointer type, which hides some miscompilations of pointer type in CIR. llvm#812 is an example. The address space in result pointer type is dropped, but the bitcast later keep the type consistency no matter what the result type is. Such bitcast is commonly emitted in CodeGen. CIR bitcasts are lowered to LLVM bitcasts, which also don't allow mismatch between address spaces. This PR adds this verification.
We were previously printing the type alias for `struct S` as `!ty_22S22`
instead of just `!ty_S`. This was because our alias computation for a
StructType did the following:
os << "ty_" << structType.getName()
`structType.getName()` is a `StringAttr`, and writing a `StringAttr` to
an output stream adds double quotes around the actual string [1][2].
These double quotes then get hex-encoded as 22 [3].
We can fix this by writing the actual string value instead. Aliases that
would end in a number will now receive a trailing underscore because of
MLIR's alias sanitization not allowing a trailing digit [4] (which
ironically didn't kick in even though our aliases were always previously
ending with a number, which might be a bug in the sanitization logic).
Aliases containing other special characters (e.g. `::`) will still be
escaped as before. In other words:
```
struct S {};
// before: !ty_22S22 = ...
// after: !ty_S = ...
struct S1 {};
// before: !ty_22S122 = ...
// after: !ty_S1_ = ...
struct std::string {};
// before: !ty_22std3A3Astring22
// after: !ty_std3A3Astring
```
I'm not a big fan of the trailing underscore special-case, but I also
don't want to touch core MLIR logic, and I think the end result is still
nicer than before.
The tests were mechanically updated with the following command run
inside `clang/test/CIR`, and the same commands can be run to update the
tests for any in-flight patches. (These are for GNU sed; for macOS
change the `-i` to `-i ''`.)
find . -type f | xargs sed -i -E -e
's/ty_22([A-Za-z0-9_$]+[0-9])22/ty_\1_/g' -e
's/ty_22([A-Za-z0-9_$]+)22/ty_\1/g'
clang/test/CIR/CodeGen/stmtexpr-init.c needed an additional minor fix to
swap the expected order of two type aliases in the CIR output.
clang/test/CIR/CodeGen/coro-task.cpp needed some surgery because it was
searching for `22` to find the end of a type alias; I changed it to
search for the actual alias instead.
If you run into merge conflicts after this change, you can auto-resolve
them via
smeenai@715f061
[1]
https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L2295
[2]
https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L2763
[3]
https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L1014
[4]
https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L1154
This PR fixes the lowering for BrCond.
Consider the following code snippet:
```
#include <stdbool.h>
bool test() {
bool x = false;
if (x)
return x;
return x;
}
```
Emitting the CIR to `tmp.cir` using `-fclangir-mem2reg` produces the
following CIR (truncated):
```
!s32i = !cir.int<s, 32>
#fn_attr = #cir<extra({inline = #cir.inline<no>, nothrow = #cir.nothrow, optnone = #cir.optnone})>
module { cir.func no_proto @test() -> !cir.bool extra(#fn_attr) {
%0 = cir.const #cir.int<0> : !s32i
%1 = cir.cast(int_to_bool, %0 : !s32i), !cir.bool
cir.br ^bb1
^bb1: // pred: ^bb0
cir.brcond %1 ^bb2, ^bb3
^bb2: // pred: ^bb1
cir.return %1 : !cir.bool
^bb3: // pred: ^bb1
cir.br ^bb4
^bb4: // pred: ^bb3
cir.return %1 : !cir.bool
}
}
```
Lowering the CIR to LLVM using `cir-opt tmp.cir -cir-to-llvm` fails
with:
```
tmp.cir:5:10: error: failed to legalize operation 'llvm.zext' marked as erased
```
The CIR cast `%1 = cir.cast(int_to_bool, %0 : !s32i)` is lowered to a
CIR comparison with zero, which is then lowered to an `LLVM::ICmpOp` and
`LLVM::ZExtOp`.
In the BrCond lowering, the zext is deleted when `zext->use_empty()`,
but during this phase the lowering for the CIR above is not complete
yet, because the zext will still have usage(s) later.
The current check for when the zext is deleted is error-prone and can be
improved.
To fix this, in addition to checking that the use of the zext is empty,
an additional check that the defining operation for the BrCond in the
CIR (the cast operation in this case) is used exactly once is added.
We haven't been able to find the root cause of llvm#829 just yet, the problem does also not show up under a ASANified build. Add some extra information before we crash, hopefully that might shed some light.
…m#822) Allow from the clang driver the use of lowering from CIR to MLIR standard dialect. Update the test to match the real output when `-fno-clangir-direct-lowering` is used, or with a combination of both `-fclangir-direct-lowering` and `-fno-clangir-direct-lowering`. --------- Co-authored-by: Bruno Cardoso Lopes <[email protected]> Co-authored-by: Shoaib Meenai <[email protected]>
We can now get the cleanup right for other potential throwing ctors, still missing LLVM lowering support.
This PR adds a new transformation that transform suitable ternary operations into select operations. Currently the "suitable" ternary operations are those ternary operations whose both branches satisfy either one of the following criteria: - The branch only contain a single `cir.yield` operation; - The branch contains a `cir.const` followed by a `cir.yield` that yields the constant value produced by the `cir.const`. - ~~The branch contains a `cir.load` followed by a `cir.yield` that yields the value loaded by the `cir.load`. The load operation cannot be volatile and must load from an alloca.~~ These criteria are hardcoded now so that simple C/C++ ternary expressions could be eventually lowered to a `cir.select` operation instead.
This is permitted by the language, and IRGen emits traps for destructors other than the base object destructor. Make CIRGen follow suit.
) The first patch to fix llvm#803 . This PR adds the calling convention attribute to CallOp directly, which is similar to LLVM, rather than adding the information to function type, which mimics Clang AST function type. The syntax of it in CIR assembly is between the function type and extra attributes, as follows: ```mlir %1 = cir.call %fnptr(%a) : (!fnptr, !s32i) -> !s32i cc(spir_kernel) extra(#fn_attr) ``` The verification of direct calls is not included. It will be included in the next patch extending CIRGen & Lowering. --- For every builder method of Call Op, an optional parameter `callingConv` is inserted right before the parameter of extra attribute. However, apart from the parser / printer, this PR does not introduce any functional changes.
… go into entry block
FlattenCFG will soon get the necessary support for lowering to LLVM, this is CIRGen only for now.
…`constructAttributeList` (llvm#831) Similar to llvm#830 , this PR completes the `setCIRFunctionAttributes` part with the call to `constructAttributeList` method, so that func op and call op share the logic of handling these kinds of attributes, which is the design of OG CodeGen. It also includes other refactors. The function `constructAttributeList` now use `mlir::NamedAttrList &` rather than immutable attribute `mlir::DictionaryAttr &` as the inout result parameter, which benefits the additive merging of attributes.
…ith OG (llvm#830) Previously the body of `setExtraAttributesForFunc` corresponds to `SetLLVMFunctionAttributesForDefinition`, but the callsite of it does not reside at the right position. This PR rename it and adjust the calls to it following OG CodeGen. To be specific, `setExtraAttributesForFunc` is called right after the initialization of `FuncOp`. But in OG CodeGen, the list of attributes is constructed by several more functions: `SetLLVMFunctionAttributes` and `SetLLVMFunctionAttributesForDefinition`. This results in diff in attributes of function declarations, which is reflected by the changes of test files. Apart from them, there is no functional change. In other words, the two code path calling `setCIRFunctionAttributesForDefinition` are tested by existing tests: * Caller `buildGlobalFunctionDefinition`: tested by `CIR/CodeGen/function-attrs.cpp`, ... * Caller `codegenCXXStructor`: tested by `CIR/CodeGen/delegating-ctor.cpp`, `defined-pure-virtual-func.cpp`, ...
This is just the usual adaption from CodeGen.
This follows the same implementation as CodeGen. llvm#1051 tracks potentially switching to a different strategy in the future.
We were missing an override for this previously and thus not emitting vtables when key functions were defined.
Upstream review of a PR requested that we be more explicit with differentiating things from MLIR to similarly named things from clang AST/LLVM/etc. So add an MLIRContext getter that we should start using. Reviewers: bcardosolopes Reviewed By: bcardosolopes Pull Request: llvm#1047
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: llvm#1021
llvm#1041) This PR adds a support for some basic cases for struct types passed by value. The hardest part probably is `createCoercedStore` function, which I rewrote significantly in order to make it closer to the orignal codegen.
This PR adds a support return struct as a value for one missed case for AArch64 big endian arch
`IntrinsicCallOp` is now named `LLVMIntrinsicCallOp` to better reflect its purpose. And now In CIR, we do not have "llvm" prefix which will be added later during LLVMLowering.
This should fix NYI like `BI__builtin_char_memchr NYI UNREACHABLE executed at clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp:1402` The test is from [OG](https://github.com/llvm/clangir/blob/3ef67c19917ad26ed8b19d4d13a43458a952fddb/clang/test/CodeGenCXX/builtins.cpp#L64) see builtin's prototype [char *__builtin_char_memchr(const char *haystack, int needle, size_t size); ](https://clang.llvm.org/docs/LanguageExtensions.html#string-builtins)
fix llvm#1057 --------- Co-authored-by: Bruno Cardoso Lopes <[email protected]> Co-authored-by: Sirui Mu <[email protected]>
…memory (llvm#1059) This PR covers one more case for return values of struct type, where `memcpy` is emitted.
This PR adds LLVMIR lowering support for `cir.assume`, `cir.assume.aligned`, and `cir.assume.separate_storage`.
In OG CodeGen, string literals has `private` linkage as default (marked by `cir_private` in CIR assembly). But CIR uses `internal`, which is probably an ancient typo. This PR keeps align with it and thus modifies the existing test files.
…bits (llvm#1068) This PR adds a partial support for so-called indirect function arguments for struct types with size > 128 bits for aarch64. #### Couple words about the implementation The hard part is that it's not one-to-one copy from the original codegen, but the code is inspired by it of course. In the original codegen there is no much job is done for the indirect arguments inside the loop in the `EmitFunctionProlog`, and additional alloca is added in the end, in the call for `EmitParamDecl` function. In our case, for the indirect argument (which is a pointer) we replace the original alloca with a new one, and store the pointer in there. And replace all the uses of the old alloca with the load from the new one, i.e. in both cases users works with the pointer to a structure. Also, I added several missed features in the `constructAttributeList` for indirect arguments, but didn't preserve the original code structure, so let me know if I need to do it.
Note that we lack two pieces of support for aliases in LLVM IR dialect globals: the `alias` keyword and function types `void (ptr)`, this needs to be done before we can nail this for good, but it's outside the scope of this commit. The behavior is slightly different under -O1, which will be addressed next.
PikachuHyA
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
bcardosolopes
requested changes
Nov 8, 2024
There was a problem hiding this comment.
This is amazing @PikachuHyA, very happy to have TBAA support in ClangIR.
|
|
||
| uint32_t g(uint32_t *s, StructA *A, uint64_t count) { | ||
| // CHECK-LABEL: define{{.*}} i32 @_Z1g | ||
| // CHECK: store i32 1, ptr %{{.*}}, align 4, !dbg !{{.*}}, !tbaa [[TAG_i32:!.*]] |
There was a problem hiding this comment.
Please remove any mentions to !dbg unless you are explicitly testing debug info. FWIW, we are currentluy emitting LLVM debug info when we shouldn't, see #793, it would be nice if that can also be fixed to prevent this annoying extra outputs!
There was a problem hiding this comment.
it would be nice if that can also be fixed to prevent this annoying extra outputs!
I would like to take on the issue #793
In my original plan, I intended to add debug info after completing support for TBAA.
There was a problem hiding this comment.
Awesome, for some reason your username is not showing up for me to assign to you, but I just gave you more repo permission to now be able to assign it yourself, let me know if doesn't work
| // with clangir enabled and some debugging info | ||
| // if disable debuggging info is supported by clangir | ||
| // the debugging info can be removed | ||
| // and keep the test case the same with clang/test/CodeGen/tbaa.c |
There was a problem hiding this comment.
Nit, let's make this comment a bit more terse (for this and other test files), example:
// This is inspired from <FILE>, with both CIR and LLVM checks.
// FIXME: use identical LLVM checks once
// https://github.com/llvm/clangir/issues/793 is fixed.
| // the debugging info can be removed | ||
| // and keep the test case the same with clang/test/CodeGen/tbaa.c | ||
|
|
||
| // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -O1 -no-struct-path-tbaa -disable-llvm-passes %s -emit-llvm -o - | FileCheck %s |
There was a problem hiding this comment.
Few additional nits:
- Please make all tests are uniform w.r.t to FileCheck prefixes (similar to what you did in
clang/test/CIR/CodeGen/tbaa3.c) - Different RUN lines for FileCheck invocations (it's usually fine to pipe directly but since this is initial support, it makes it way more convenient to review when running tests and clicking at an output file, instead of having to do that by hand when trying to look at the full output).
Example for this specific case: LLVM_NO_STRUCT and LLVM_STRUCT
|
|
||
| // Test TBAA metadata generated by front-end. | ||
| // | ||
| // NO-TBAA-NOT: !tbaa |
There was a problem hiding this comment.
What's this NO-TBAA-NOT about? Remove it here and in the other test
|
|
||
| mlir::LLVM::TBAARootAttr root; |
There was a problem hiding this comment.
Please remove all member repetions in the comments, i.e. StructMetadataCache - and all others. LLVM deprecated this already and we don't want to generate extra burden when upstreaming. This should apply everywhere else in this PR, not only this file.
| @@ -3525,8 +3533,77 @@ void CIRGenModule::buildGlobalAnnotations() { | |||
| deferredAnnotations.clear(); | |||
| } | |||
|
|
|||
| mlir::LLVM::TBAATypeDescriptorAttr CIRGenModule::getTBAATypeInfo(QualType QTy) { | |||
There was a problem hiding this comment.
We don't want to use LLVM IR Dialect specific stuff, like mlir::LLVM::TBAATypeDescriptorAttr directly in CIRGen or anywhere before LowerToLLVM.
The idea is to design our CIR version of this, even if very similar. I wonder if we really need to pass in the same amount of information LLVM requires though, because we have the types until lowering time, perhaps we could do something a bit more terse and expand to the full metadata content during LLVM lowering? For example, we don't need to encode a string for id = "omnipotent char" while in CIR, that could be just a enum kind if we need to encode it early (do we tho?). The overall idea is to infer things that we can during lowering while only encoding information during CIRGen that it's hard to do later on. Note that CIR can be lowered to non-LLVM targets, so we need to be a bit strategical here.
Also, the tbaa information should be first class into cir.load/cir.store, so {tbaa = [#tbaa_tag]} should probably just be tbaa([#tbaa_tag]) or something that's not only dumped into the generic attributes space.
This is a big PR that touches lots of things, I think we should slightly break this up so we can land the bulk faster and spend time on a PR for the specific attribute design. My suggestion is that first PR should just add the overall skeleton for CIRGen, this effectively means changing this PR such that:
- Create a dummy
mlir::cir::TBAA_Attr, shouldn't have anything in it. - Replace all LLVM specific attribute bits like
mlir::LLVM::TBAATypeDescriptorAttr, tomlir::cir::TBAA_Attr. - The current implementation of everything that returns
mlir::cir::TBAA_Attrshould just return amlir::cir::TBAA_Attr. - Attach to loads/stores just like you are currently doing.
- One new really simple CIR test that checks the presence of
mlir::cir::TBAA_Attron cir.load/cir.store. - No LLVM lowering.
There was a problem hiding this comment.
Thanks for your comments. I completely agree with your points about not using LLVM-specific elements in CIRGen. It's important to keep things clean for potential non-LLVM targets.
I will break down the PR as you suggested.
| @@ -0,0 +1,121 @@ | |||
| // modified from clang/test/CodeGen/tbaa.c | |||
There was a problem hiding this comment.
Thanks for the comprehensive tests! Can you please rename them to be a bit more descriptive? Example: if this is only dealing with enums, should tbaa-enum.c. Also, if the test is only checking for LLVM output, it should be under Lowering, if it's testing both CIR and LLVM, than CodeGen is good enough.
|
|
||
| struct tuple_t { | ||
| intkey_t key; | ||
| value_t payload; |
There was a problem hiding this comment.
I'm fine with any indentation on tests, but they should be consistent within one file, looks like this differs from the next struct
bcardosolopes
pushed a commit
that referenced
this pull request
Nov 19, 2024
This is the first patch to support TBAA, following the discussion at #1076 (comment) - add skeleton for CIRGen, utilizing `decorateOperationWithTBAA` - add empty implementation in `CIRGenTBAA` - introduce `CIR_TBAAAttr` with empty body - attach `CIR_TBAAAttr` to `LoadOp` and `StoreOp` - no handling of vtable pointer - no LLVM lowering
smeenai
force-pushed
the
main
branch
2 times, most recently
from
4aca8d4
to
a04cf10
Compare
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 patch introduces support for TBAA, following the structure of
clang/lib/CodeGen/CodeGenTBAA.h. The key function implemented isCIRGenModule::decorateOperationWithTBAA, which works similarly toCodeGenModule::DecorateInstructionWithTBAA.Note: Support for
vtable pointerandtbaa.structis not yet included.For further details, please refer to: