-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[CIR] Upstream ArraySubscriptExpr for fixed size array #134536
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
Changes from 3 commits
81e1cd7
8d2acb1
e7d093b
e069e0e
ac0e8c3
33eff17
9d07a29
abd9621
bd7f0cc
fb439ca
e6fe258
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
| // See https://llvm.org/LICENSE.txt for license information. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #include "CIRGenBuilder.h" | ||
|
|
||
| using namespace clang::CIRGen; | ||
|
|
||
| mlir::Value CIRGenBuilderTy::maybeBuildArrayDecay(mlir::Location loc, | ||
| mlir::Value arrayPtr, | ||
| mlir::Type eltTy) { | ||
| const auto arrayPtrTy = mlir::cast<cir::PointerType>(arrayPtr.getType()); | ||
| const auto arrayTy = mlir::dyn_cast<cir::ArrayType>(arrayPtrTy.getPointee()); | ||
|
|
||
| if (arrayTy) { | ||
| const cir::PointerType flatPtrTy = getPointerTo(arrayTy.getEltType()); | ||
| return create<cir::CastOp>(loc, flatPtrTy, cir::CastKind::array_to_ptrdecay, | ||
| arrayPtr); | ||
| } | ||
|
|
||
| assert(arrayPtrTy.getPointee() == eltTy && | ||
| "flat pointee type must match original array element type"); | ||
| return arrayPtr; | ||
| } | ||
|
|
||
| mlir::Value CIRGenBuilderTy::getArrayElement(mlir::Location arrayLocBegin, | ||
| mlir::Location arrayLocEnd, | ||
| mlir::Value arrayPtr, | ||
| mlir::Type eltTy, mlir::Value idx, | ||
| bool shouldDecay) { | ||
| mlir::Value basePtr = arrayPtr; | ||
| if (shouldDecay) | ||
| basePtr = maybeBuildArrayDecay(arrayLocBegin, arrayPtr, eltTy); | ||
| const mlir::Type flatPtrTy = basePtr.getType(); | ||
| return create<cir::PtrStrideOp>(arrayLocEnd, flatPtrTy, basePtr, idx); | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |||||||
|
|
||||||||
| #include "Address.h" | ||||||||
| #include "CIRGenFunction.h" | ||||||||
| #include "CIRGenModule.h" | ||||||||
| #include "CIRGenValue.h" | ||||||||
| #include "mlir/IR/BuiltinAttributes.h" | ||||||||
| #include "clang/AST/Attr.h" | ||||||||
|
|
@@ -232,6 +233,152 @@ LValue CIRGenFunction::emitUnaryOpLValue(const UnaryOperator *e) { | |||||||
| llvm_unreachable("Unknown unary operator kind!"); | ||||||||
| } | ||||||||
|
|
||||||||
| /// If the specified expr is a simple decay from an array to pointer, | ||||||||
| /// return the array subexpression. | ||||||||
| /// FIXME: this could be abstracted into a common AST helper. | ||||||||
| static const Expr *getSimpleArrayDecayOperand(const Expr *e) { | ||||||||
| // If this isn't just an array->pointer decay, bail out. | ||||||||
| const auto *castExpr = dyn_cast<CastExpr>(e); | ||||||||
| if (!castExpr || castExpr->getCastKind() != CK_ArrayToPointerDecay) | ||||||||
| return nullptr; | ||||||||
|
|
||||||||
| // If this is a decay from variable width array, bail out. | ||||||||
| const Expr *subExpr = castExpr->getSubExpr(); | ||||||||
| if (subExpr->getType()->isVariableArrayType()) | ||||||||
| return nullptr; | ||||||||
|
|
||||||||
| return subExpr; | ||||||||
| } | ||||||||
|
|
||||||||
| static mlir::IntegerAttr getConstantIndexOrNull(mlir::Value idx) { | ||||||||
|
||||||||
| static mlir::IntegerAttr getConstantIndexOrNull(mlir::Value idx) { | |
| static cir::IntAttr getConstantIndexOrNull(mlir::Value idx) { |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this ever return non-null? I would expect the constantOp to have a cir::IntAttr rather than an mlir::IntegerAttr. I'm not sure the former can be cast to the latter.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return mlir::dyn_cast<mlir::IntegerAttr>(constantOp.getValue()); | |
| return mlir::dyn_cast<cir::IntAttr>(constantOp.getValue()); |
I just tested this in the incubator, and as I suspected, this function was always returning null because our constants don't use mlir::IntegerAttr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the function signature and cast, I will understand this part more in the incubator because that means it was only depend on that condition (!CGF.IsInPreservedAIRegion && !isPreserveAIArrayBase(CGF, Base)) not the index itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking to upstream at least isPreserveAIArrayBase because now even this NYI message is wrong, it should be related to PreserveAIArray so I think it better to upstream isPreserveAIArrayBase so I can make the else branch related to itcgf.cgm.errorNYI("emitArraySubscriptExpr: Non Constant Index");
And in that case will add test to cover array[x]
What do you think? @andykaylor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, cgf.isInPreservedAIRegion will never be true until we support the associated builtin function (__builtin_preserve_access_index), which isn't supported yet in the incubator and isn't a high priority and might not be relevant to CIR at all. I don't see any reason to upstream any of that yet.
But you're right that the check in emitArraySubscriptPtr is wrong as it currently appears in this PR. It appears that there is no reason to be checking for a constant index as we are there and we should be unconditionally calling the code in the true case. If you can support a variable index with no other changes, go ahead. Otherwise, leave it for a subsequent patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andykaylor That makes sense, I updated the test to include an array with a variable index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test case where this would return something different than alignmentOfArrayElement(eltSize)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking for test case that can be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to find a test case for this, I modified the equivalent classic clang codegen implementation so that it would always go to the else case here. That caused 15 tests to fail, but we don't have enough support implemented to run any of the failing tests with CIR. There were 12 OpenMP failures, two PowerPC intrinsics, and one HLSL.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signedIndices parameter isn't needed here.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assert(!cir::MissingFeatures::emitCheckedInBoundsGEP() && "NYI"); | |
| assert(!cir::MissingFeatures::emitCheckedInBoundsGEP()); |
Also, there's no need for this to be conditional.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arrayType and base parameters aren't referenced. They can be removed until they are needed.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The incubator is also checking (!CGF.IsInPreservedAIRegion && !isPreserveAIArrayBase(CGF, Base))) here. Can you add a MissingFeatures assert for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I can easily add MissingFeatures without conflict with the else branch, maybe I should upstream the basic implementation of this failed and function with errorNYI in their cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MissingFeatures assert is effectively a noop. You're asserting something which is statically true. It just serves as a placeholder in the code to help us find places where changes need to be made in the future.
| if (!index) { | |
| if (!index) { | |
| assert(!cir::MissingFeatures::preservedAccessIndexRegion()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats right, thank you @andykaylor
andykaylor marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,8 +29,24 @@ int f[5] = {1, 2}; | |
|
|
||
| void func() { | ||
| int arr[10]; | ||
| int e = arr[0]; | ||
| int e2 = arr[1]; | ||
|
|
||
| // CHECK: %[[ARR:.*]] = cir.alloca !cir.array<!s32i x 10>, !cir.ptr<!cir.array<!s32i x 10>>, ["arr"] | ||
| // CHECK: %[[INIT:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, ["e", init] | ||
| // CHECK: %[[INIT_2:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, ["e2", init] | ||
|
|
||
| // CHECK: %[[IDX:.*]] = cir.const #cir.int<0> : !s32i | ||
| // CHECK: %[[ARR_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR]] : !cir.ptr<!cir.array<!s32i x 10>>), !cir.ptr<!s32i> | ||
| // CHECK: %[[ELE_PTR:.*]] = cir.ptr_stride(%[[ARR_PTR]] : !cir.ptr<!s32i>, %[[IDX]] : !s32i), !cir.ptr<!s32i> | ||
| // CHECK: %[[TMP:.*]] = cir.load %[[ELE_PTR]] : !cir.ptr<!s32i>, !s32i | ||
| // CHECK" cir.store %[[TMP]], %[[INIT]] : !s32i, !cir.ptr<!s32i> | ||
|
||
|
|
||
| // CHECK: %[[IDX:.*]] = cir.const #cir.int<1> : !s32i | ||
| // CHECK: %[[ARR_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR]] : !cir.ptr<!cir.array<!s32i x 10>>), !cir.ptr<!s32i> | ||
| // CHECK: %[[ELE_PTR:.*]] = cir.ptr_stride(%[[ARR_PTR]] : !cir.ptr<!s32i>, %[[IDX]] : !s32i), !cir.ptr<!s32i> | ||
| // CHECK: %[[TMP:.*]] = cir.load %[[ELE_PTR]] : !cir.ptr<!s32i>, !s32i | ||
| // CHECK" cir.store %[[TMP]], %[[INIT_2]] : !s32i, !cir.ptr<!s32i> | ||
| } | ||
|
|
||
| void func2() { | ||
|
|
@@ -69,6 +85,7 @@ void func4() { | |
| int arr[2][1] = {{5}, {6}}; | ||
|
|
||
| // CHECK: %[[ARR:.*]] = cir.alloca !cir.array<!cir.array<!s32i x 1> x 2>, !cir.ptr<!cir.array<!cir.array<!s32i x 1> x 2>>, ["arr", init] | ||
| // CHECK: %[[INIT:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, ["e", init] | ||
| // CHECK: %[[ARR_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR]] : !cir.ptr<!cir.array<!cir.array<!s32i x 1> x 2>>), !cir.ptr<!cir.array<!s32i x 1>> | ||
| // CHECK: %[[ARR_0_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR_PTR]] : !cir.ptr<!cir.array<!s32i x 1>>), !cir.ptr<!s32i> | ||
| // CHECK: %[[V_0_0:.*]] = cir.const #cir.int<5> : !s32i | ||
|
|
@@ -78,6 +95,17 @@ void func4() { | |
| // CHECK: %[[ARR_1_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR_1]] : !cir.ptr<!cir.array<!s32i x 1>>), !cir.ptr<!s32i> | ||
| // CHECK: %[[V_1_0:.*]] = cir.const #cir.int<6> : !s32i | ||
| // CHECK: cir.store %[[V_1_0]], %[[ARR_1_PTR]] : !s32i, !cir.ptr<!s32i> | ||
|
|
||
| int e = arr[1][0]; | ||
|
|
||
| // CHECK: %[[IDX:.*]] = cir.const #cir.int<0> : !s32i | ||
| // CHECK: %[[IDX_1:.*]] = cir.const #cir.int<1> : !s32i | ||
| // CHECK: %[[ARR_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR]] : !cir.ptr<!cir.array<!cir.array<!s32i x 1> x 2>>), !cir.ptr<!cir.array<!s32i x 1>> | ||
| // CHECK: %[[ARR_1:.*]] = cir.ptr_stride(%[[ARR_PTR]] : !cir.ptr<!cir.array<!s32i x 1>>, %[[IDX_1]] : !s32i), !cir.ptr<!cir.array<!s32i x 1>> | ||
| // CHECK: %[[ARR_1_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR_1]] : !cir.ptr<!cir.array<!s32i x 1>>), !cir.ptr<!s32i> | ||
| // CHECK: %[[ELE_0:.*]] = cir.ptr_stride(%[[ARR_1_PTR]] : !cir.ptr<!s32i>, %[[IDX]] : !s32i), !cir.ptr<!s32i> | ||
| // CHECK: %[[TMP:.*]] = cir.load %[[ELE_0]] : !cir.ptr<!s32i>, !s32i | ||
| // CHECK: cir.store %[[TMP]], %[[INIT]] : !s32i, !cir.ptr<!s32i> | ||
| } | ||
|
|
||
| void func5() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need an Assert/NYI/etc here? Seems like the purpose of MissingFeatures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, it's internal to be nullptr, not a missing feature or implementation similar to Clang
llvm-project/clang/lib/CodeGen/CGExpr.cpp
Lines 4054 to 4068 in 783201b