[CIR] Replace uses of global with alias #1901
Draft
+166,758
−44,635
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
Initial implementation of lowering cir.delete.array llvm#1285 lowered to a call to **_ZdaPv** keeping similarity with how Clang AST is lowered. before ```LLVM module @"/opt/tmp/input.cpp" attributes {cir.lang = #cir.lang<cxx>, cir.sob = #cir.signed_overflow_behavior<undefined>, cir.triple = "x86_64-unknown-linux-gnu", cir.type_size_info = #cir.type_size_info<char = 8, int = 32, size_t = 64>, dlti.dl_spec = #dlti.dl_spec<!llvm.ptr<271> = dense<32> : vector<4xi64>, !llvm.ptr<272> = dense<64> : vector<4xi64>, i128 = dense<128> : vector<2xi64>, !llvm.ptr<270> = dense<32> : vector<4xi64>, f80 = dense<128> : vector<2xi64>, i1 = dense<8> : vector<2xi64>, !llvm.ptr = dense<64> : vector<4xi64>, i8 = dense<8> : vector<2xi64>, i16 = dense<16> : vector<2xi64>, i32 = dense<32> : vector<2xi64>, f64 = dense<64> : vector<2xi64>, f128 = dense<128> : vector<2xi64>, f16 = dense<16> : vector<2xi64>, i64 = dense<64> : vector<2xi64>, "dlti.stack_alignment" = 128 : i64, "dlti.endianness" = "little", "dlti.mangling_mode" = "e">} { cir.func @_Z17test_delete_arrayPi(%arg0: !cir.ptr<!s32i> loc(fused[#loc3, #loc4])) extra(#fn_attr) { %0 = cir.alloca !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>>, ["ptr", init] {alignment = 8 : i64} loc(#loc8) cir.store %arg0, %0 : !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>> loc(#loc5) %1 = cir.load %0 : !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!s32i> loc(#loc6) cir.delete.array %1 : <!s32i> loc(#loc6) cir.return loc(#loc2) } loc(#loc7) } loc(#loc) ``` after ```LLVM define dso_local void @_Z17test_delete_arrayPi(ptr %0) #0 { %2 = alloca ptr, i64 1, align 8 store ptr %0, ptr %2, align 8 %3 = load ptr, ptr %2, align 8 call void @_ZdaPv(ptr %3) ret void } ```
…llvm#1605) - Rename `PrimitiveIntOrFPPtr` to `CIR_PtrToIntOrFloatType` and broaden it to accept any floating-point type. - Fix incorrect constraint name from `PrimitiveInt` to any `Int`; prior constraint already allowed arbitrary bitwidths, so the name was misleading. - Rename `ComplexPtr` to `CIR_PtrToComplexType` for clarity.
Remove all the code the manages ABI info for arguments and return values in the initial CIR codegen. We prefer for the CIR to represent the types as they appear in the source code, with ABI handling being deferred until the lowering phase or calling convention transform. The ABI handling being removed here was brought over from the classic codegen, but none of the effects being computed made it into the CIR so this change is effectively NFC. This change leaves the `CIRGenFunctionInfoArgInfo` with just one member. That can be cleaned up in a later patch.
There are some cases where we're currently using the zero attribute to initialize global variables for records that aren't properly zero-initializable. We weren't checking for that, and we also weren't properly calculating zero-initializability for records. This patch addresses both of these issues. This doesn't actually handle the case where the record isn't zero-initializable. It just reports it as NYI. It does add a comment about what needs to be done.
Backport #139304
Currently, the following code snippet fails with a crash:
```
#include <stdio.h>
struct X {
int a;
X(int a) : a(a) {}
~X() {}
};
bool foo(const X &) { return false; }
bool bar() { return foo(1) || foo(2); }
```
it fails with
```
error: 'cir.yield' op must be the last operation in the parent block
```
The crash can be traced to the [construction of the
TernaryOp](https://github.com/llvm/clangir/blob/5df50096be1a783c26b48ea437523347df8a3110/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp#L2728).
A
[yield](https://github.com/llvm/clangir/blob/5df50096be1a783c26b48ea437523347df8a3110/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp#L2776)
is created and when the LexicalScope is destroyed the destructors for
the object follow.
So, the CIR looks something like:
```
%11 = "cir.ternary"(%10) ({
%13 = "cir.const"() <{value = #cir.bool<true> : !cir.bool}> : () -> !cir.bool
"cir.yield"(%13) : (!cir.bool) -> ()
}, {
%12 = "cir.const"() <{value = #cir.bool<false> : !cir.bool}> : () -> !cir.bool
"cir.yield"(%12) : (!cir.bool) -> ()
}) : (!cir.bool) -> !cir.bool
"cir.yield"(%11) : (!cir.bool) -> ()
"cir.call"(%7) <{callee = @_ZN1XD2Ev, calling_conv = 1 : i32, extra_attrs = #cir<extra({nothrow = #cir.nothrow})>, side_effect = 1 : i32}> ({
})
```
which is wrong, since the yield should come last.
The fix here is forcing a cleanup and then the yield follows. A similar
rule also applies
[here](https://github.com/llvm/clangir/blob/5df50096be1a783c26b48ea437523347df8a3110/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp#L2657)
for the "&&" case, e.g., foo(1) && foo(2) instead in the code snippet.
One more modification I made is adding a check for when the current
lexScope is ternary, when creating dispatch blocks for cleanups. I
believe in this case, we do not want to create a dispatch block, please
correct me if I am wrong.
Finally, I add two tests to verify that everything works as intended.
A previous refactoring had reduced the ArgInfo structure to contain a single member, the argument type. This change eliminates the ArgInfo structure entirely, instead just storing the argument type directly in places where ArgInfo had previously been used.
Add mlir Vec support to elementTypeIfVector
This commit un-xfails some tests that were affected by upstream GEP changes. llvm#1497 The changes are likely introduced by inference of `inbounds` and `nuw` flags llvm/llvm-project@10f315d. It seems to allow LLVM to enable some constant foldings, whose effects include re-calculating the address using i8 and a single offset. (This change seems to arise from https://discourse.llvm.org/t/rfc-replacing-getelementptr-with-ptradd/68699/39)
Reapplying llvm@e66b670 which was reverted during rebase (after fixing some conflicts). Un-xfails the test `cc1.cir` (llvm#1497). --------- Co-authored-by: Nathan Lanza <[email protected]>
This commit attempts to un-xfail `globals-ref-globals` test (llvm#1497), which was causing a crash in addition to GEP changes. The crash was caused by incorrect offsets being calculated for global view indices. The original calculation did not take paddings into consideration, hence triggering a crash. This commit adds type alignment when calculating the offset, which will take care of paddings. Further modification to `globals-ref-globals` test includes fixing some struct names that got changed, as well as replacing the expects with constant folded GEP.
Previously, when emitting a global for a string literal, we were creating a GlobalOp, building a GlobalView attr for it, and looking up the global from the symbol associated with the attr. This change splits out the function that creates the global so that the global is returned directly and the GlobalView attribute is only created in the case where it is needed. This also updates the mechanism used for uniquing the global name used for the strings so that if different base names are used the uniquing numbers each base name separately. The mangling of the global used for strings is not implemented, but the uniquing was happening prior to the mangling. This change drops the uniquing below the placeholder for mangling.
This change corrects the alignment of store operations and fixes a related problem with calculation of member offsets (we weren't accounting for the alignment of the field whose offset we were calculating. Many tests are affected by this, but most just needed a wildcard match to ignore the explicit alignment which wasn't present before. In cases where I updated a check for a specific alignment value, I compared against classic codegen to verify that we are now producing the same alignment. Two new tests are added align-store.c and alignment.cpp. The second of these partially copies a test of the same name from clang/test/CodeGen. It's testing globals and isn't directly related to the code changes here, but we didn't seem to have a test for this. I put the store alignment tests in a different file because inconsistency between CIR and LLVM IR in placement of globals would have made a combined test difficult to follow. This addresses llvm#1204
Currently the ForOp handling ignores everything except load, store and
arithmetic in the step region. It does not detect whether the step and
induction variable has already been assigned, either.
That might result to wrong behaviour:
```cpp
// Ignores printf
for (int i = 0; i < n; i++, printf("\n"));
// Only increments once
for (int i = 0; i < n; i++, i++);
```
I choose to rewrite the detection and do an exact match of the
instruction sequence for `i++` and `i += n`. It doesn't seem easy to
detect a more general pattern without phi nodes.
The new test case is xfailed, because ForOp hits an unreachable when it
meets a non-canonicalized loop. We can implement that functionality
later.
Co-authored-by: Yue Huang <[email protected]>
Implement Atomic init for ComplexType
Implement VisitOpaqueValueExpr for ComplexType
`cir::BreakOp::getBreakTarget` gets the innermost `cir::LoopOpInterface`
or `cir::SwitchOp` containing this `break`.
For example:
```
A: for (...) {
B: for(...) {
break; // target = B
C: switch (...) {
default: break; // target = C
}
break; // target = A
}
```
NOTE: This is a part of a broader effort I am working on to make
querying CIR control flow facts more easily. If folks have any design
notes or ideas, please share them.
---------
Co-authored-by: Tommy McMichen <[email protected]>
**Related Issue**: llvm#1880
This implements getting the VTT argument for a delegating constructor and enables the corresponding test in delegating-ctor.cpp.
First noted at: llvm#1829 Also: Adjusted test cases affected by this change. --------- Co-authored-by: Tommy MᶜMichen <[email protected]>
This reverts commit a896c41. Test is failing on Linux (reproduced locally)
This reverts commit 4e092b2.
Three things: - Corrected comments to `getZeroInitAttr` as [we return more than only integrals in that function](https://github.com/llvm/clangir/blob/2ea4005fa0aa291295b19c200860b5edf9b864b3/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h#L133). - Given that `emitX86MaskedCompare` and `emitX86MaskedCompareResult` helpers are pretty large, Added NYI statements on paths not related to the current set of intrinsics so review is specific to the ones encoded. - Added test comments related to the behavior observed coming from the canonicalizer on: llvm#1770
llvm#1887) Adds `cir::CtorKind::Move` to `CXXCtorAttr` for move constructors Add `cir::CXXAssignAttr` with `cir::AssignKind::{Move,Copy}` for move/copy assignment operators Per discussion in llvm#1879 Updated lifetime checker to use the `CXXAssignAttr` instead of AST interface. I will be making a PR that depends on `cir::CXXCtorKind::Move` soon. --------- Co-authored-by: Tommy McMichen <[email protected]> Co-authored-by: Tommy McMichen <[email protected]>
From the [comment](llvm#1844 (comment)) on PR review llvm#1844, it seems like we're missing the flags for GEP. I'm opening the PR to add the flags. The first commit is just a prototype to gather opinions and reviews to see if I'm heading to the right direction with this.
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp -- clang/lib/CIR/CodeGen/CIRGenModule.cpp mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cppView the diff from clang-format here.diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index bc81f93b7b..ebfa6a6b0b 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -1184,7 +1184,7 @@ LogicalResult CallOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
fnType = fn.getFunctionType();
} else if (auto ifunc = dyn_cast<IFuncOp>(callee)) {
fnType = ifunc.getIFuncType();
- } else if (auto aliasFn = dyn_cast<AliasOp>(callee)){
+ } else if (auto aliasFn = dyn_cast<AliasOp>(callee)) {
fnType = aliasFn.getType();
} else {
return emitOpError()
|
Contributor
Author
|
i'm putting up a temp commit to gather reviews if i'm heading in the right direction. Right now the implementation is hitting a verification error on the following test case but i'm not really sure why the LLVM IR module before lowering to LLVM and the LLVM module before verification is this. |
lanza
force-pushed
the
main
branch
2 times, most recently
from
8acaf96 to
58135ea
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
26 participants
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.
Fixes #1809