[mlir][llvm] Add zeroinitializer constant #65508

sitio-couto · 2023-09-06T17:52:37Z

This patch adds support for the zeroinitializer constant to LLVM dialect. It's meant to simplify zero-initialization of aggregate types in MLIR, although it can also be used with non-aggregate types.

The LLVM Dialect in MLIR, which the `mlir-llvm` team is supposed to provide notifications for, is 98% not nested in a directory called LLVM but rather LLVMIR. The former only contains some tests. This should make PRs such as llvm#65508 add the team as codeowner.

gysit

Thanks for the very thorough testing!

I would nevertheless suggest to reduce the test set a bit :). After removing the typed pointers some tests will not have a lot of value (e.g. the function pointer test). Additionally, you do not need to check every type in the context of a global and in the context of a function.

After all we are mostly testing an LLVM function (if I understand correctly):
llvm::Constant::getNullValue($_resultType)
plus the type conversion applied to convert the result type. This type conversion should already be tested though.

I would suggest to keep some tests with global variables and some tests in the context of the function and using different types for all of them. Having some more complex aggregate type for sure is nice as well.

gysit · 2023-09-06T18:54:20Z

mlir/test/Target/LLVMIR/llvmir.mlir

+}
+// CHECK: @zero_struct = linkonce global { i32, double, i8 } zeroinitializer
+
+llvm.mlir.global linkonce @zero_ptr() : !llvm.ptr<i32> {


Can you avoid typed pointers and instead always use opaque pointers (opaque pointers don't have an element type and look as follows:

Suggested change

llvm.mlir.global linkonce @zero_ptr() : !llvm.ptr<i32> {

llvm.mlir.global linkonce @zero_ptr() : !llvm.ptr {

LLVM does not have typed pointers anymore and MLIR is in a transition phase:
https://discourse.llvm.org/t/rfc-switching-the-llvm-dialect-and-dialect-lowerings-to-opaque-pointers/68179

We would thus like to avoid any new uses of typed pointers.

Oh, nice. Wasn't aware of that. Typed pointers removed!

gysit · 2023-09-06T18:59:29Z

mlir/test/Target/LLVMIR/llvmir.mlir

+  %local_integer = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr<i32>
+  %1 = llvm.mlir.zero : i32
+  llvm.store %1, %local_integer : !llvm.ptr<i32>
+  // CHECK: %[[#V1:]] = alloca i32, i64 1, align 4
+  // CHECK: store i32 0, ptr %[[#V1]], align 4


Suggested change

%local_integer = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr<i32>

%1 = llvm.mlir.zero : i32

llvm.store %1, %local_integer : !llvm.ptr<i32>

// CHECK: %[[#V1:]] = alloca i32, i64 1, align 4

// CHECK: store i32 0, ptr %[[#V1]], align 4

// CHECK: %[[#V1:]] = alloca i32, i64 1, align 4

%local_integer = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr<i32>

%1 = llvm.mlir.zero : i32

// CHECK: store i32 0, ptr %[[#V1]], align 4

llvm.store %1, %local_integer : !llvm.ptr<i32>

ultra nit: We usually prefer to put a file check comment right above the statement it matches. This interspersed view is a bit easier to read especially for larger tests.

gysit · 2023-09-06T19:39:43Z

One last comment. You may want to do add a short roundtrip test to test printing and parsing, similar to this one:

llvm-project/mlir/test/Dialect/LLVMIR/roundtrip.mlir

Line 333 in 8be07ad

%0 = llvm.mlir.null : !llvm.ptr

sitio-couto · 2023-09-06T22:09:39Z

Thanks for the review!

I would nevertheless suggest to reduce the test set a bit :)

I've reduced quite a bit (down to 2 tests only) since there was a lot of redundancy.

gysit

Thanks for the changes!

LGTM

ftynse

LGTM.

How does this relate to llvm.mlir.null? Can we also initialize pointers with llvm.mlir.zero and remove llvm.mlir.null in favor of the more general operation?

ftynse · 2023-09-07T09:34:57Z

mlir/test/Target/LLVMIR/llvmir.mlir

+}
+
+llvm.func @zeroinit_complex_local_aggregate() {
+  // CHECK: %[[#VAR:]] = alloca [1000 x { i32, [3 x { double, <4 x ptr>, [2 x ptr] }], [6 x ptr] }], i64 1, align 32


I'm curious, how does %[[#VAR:]] work? Usually, we are using %[[#VAR:.*]] to capture the name.

It's called FileCheck Numeric Substitution Blocks! The # prefix enables it. It can be used to check more complex numeric relations, but it can also check basic numeric equalities.

gysit · 2023-09-07T10:55:20Z

Can we also initialize pointers with llvm.mlir.zero and remove llvm.mlir.null in favor of the more general operation?

That is a good point! I think that would be a nice cleanup that probably can be done in a follow up revision.

sitio-couto · 2023-09-07T11:23:35Z

How does this relate to llvm.mlir.null?

The llvm.mlir.zero is essentially more generic. It can work with most LLVM types (aggregate or not) as far as I tested.

Can we also initialize pointers with llvm.mlir.zero and remove llvm.mlir.null in favor of the more general operation?

Yes! On the builder side, it should behave exactly the same, since llvm::Constant::getNullValue will call llvm::ConstantPointerNull::get. But since llvm.mlir.null implies a pointer (e.g. return type is always a pointer), there might be API calls and passes that will have to be updated to work with the more generic llvm.mlir.zero op.

The LLVM Dialect in MLIR, which the `mlir-llvm` team is supposed to provide notifications for, is 98% not nested in a directory called LLVM but rather LLVMIR. The former only contains some tests. This should make PRs such as #65508 add the team as codeowner.

zero9178

@sitio-couto do you need one of use to merge this PR for you?

sitio-couto · 2023-09-08T10:45:02Z

Do you need one of us to merge this PR for you?

@zero9178 yes, please. It should auto-rebase without conflicts, but I can also rebase it manually if necessary.

zero9178 · 2023-09-08T11:06:27Z

Do you need one of us to merge this PR for you?

@zero9178 yes, please. It should auto-rebase without conflicts, but I can also rebase it manually if necessary.

No worries, all good! I also tested it locally ontop of HEAD before merging

This revision replaces the LLVM dialect NullOp by the recently introduced ZeroOp. The ZeroOp is more generic in the sense that it represents zero values of any LLVM type rather than null pointers only. This is a follow to llvm#65508

This revision replaces the LLVM dialect NullOp by the recently introduced ZeroOp. The ZeroOp is more generic in the sense that it represents zero values of any LLVM type rather than null pointers only. This is a follow to #65508

This is not standard but is vastly expected by existing code. This was implemented by https://reviews.llvm.org/D149877 for simple scalars, but MLIR lacked a generic way to deal with aggregate types (arrays and derived type). Support was recently added in llvm#65508. Leverage it to zero initialize all types.

) This is not standard but is vastly expected by existing code. This was implemented by https://reviews.llvm.org/D149877 for simple scalars, but MLIR lacked a generic way to deal with aggregate types (arrays and derived type). Support was recently added in #65508. Leverage it to zero initialize all types.

…m#67693) This is not standard but is vastly expected by existing code. This was implemented by https://reviews.llvm.org/D149877 for simple scalars, but MLIR lacked a generic way to deal with aggregate types (arrays and derived type). Support was recently added in llvm#65508. Leverage it to zero initialize all types.

The LLVM Dialect in MLIR, which the `mlir-llvm` team is supposed to provide notifications for, is 98% not nested in a directory called LLVM but rather LLVMIR. The former only contains some tests. This should make PRs such as llvm/llvm-project#65508 add the team as codeowner.

[mlir][llvm] Add zeroinitializer constant

89cf3f8

This patch adds support for the zeroinitializer constant to LLVM dialect. It's meant to simplify zero-initialization of aggregate types in MLIR, although it can also be used with non-aggregate types.

sitio-couto requested a review from a team as a code owner September 6, 2023 17:52

github-actions bot added the mlir label Sep 6, 2023

zero9178 requested review from ftynse, zero9178, gysit and Dinistro September 6, 2023 18:28

zero9178 mentioned this pull request Sep 6, 2023

[GitHub] Add main LLVM Dialect path to MLIR LLVM PR subscribers #65513

Merged

gysit reviewed Sep 6, 2023

View reviewed changes

Apply first round of reviews

a82a416

Fix test FileCheck variable usage

ecedd7b

gysit approved these changes Sep 7, 2023

View reviewed changes

ftynse approved these changes Sep 7, 2023

View reviewed changes

zero9178 approved these changes Sep 8, 2023

View reviewed changes

zero9178 merged commit 01192a0 into llvm:main Sep 8, 2023

michaelrj-google mentioned this pull request Sep 12, 2023

[libc] Move long double table option to new config #66151

Merged

gysit mentioned this pull request Sep 22, 2023

[mlir][llvm] Replace NullOp by ZeroOp #67183

Merged

jeanPerier mentioned this pull request Sep 28, 2023

[flang] zero initialized all saved values without initial values #67693

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mlir][llvm] Add zeroinitializer constant #65508

[mlir][llvm] Add zeroinitializer constant #65508

sitio-couto commented Sep 6, 2023

gysit left a comment

gysit Sep 6, 2023

sitio-couto Sep 6, 2023

gysit Sep 6, 2023

gysit commented Sep 6, 2023

sitio-couto commented Sep 6, 2023

gysit left a comment

ftynse left a comment

ftynse Sep 7, 2023

sitio-couto Sep 7, 2023

gysit commented Sep 7, 2023

sitio-couto commented Sep 7, 2023

zero9178 left a comment

sitio-couto commented Sep 8, 2023

zero9178 commented Sep 8, 2023

	llvm.mlir.global linkonce @zero_ptr() : !llvm.ptr<i32> {
	llvm.mlir.global linkonce @zero_ptr() : !llvm.ptr {

[mlir][llvm] Add zeroinitializer constant #65508

[mlir][llvm] Add zeroinitializer constant #65508

Conversation

sitio-couto commented Sep 6, 2023

gysit left a comment

Choose a reason for hiding this comment

gysit Sep 6, 2023

Choose a reason for hiding this comment

sitio-couto Sep 6, 2023

Choose a reason for hiding this comment

gysit Sep 6, 2023

Choose a reason for hiding this comment

gysit commented Sep 6, 2023

sitio-couto commented Sep 6, 2023

gysit left a comment

Choose a reason for hiding this comment

ftynse left a comment

Choose a reason for hiding this comment

ftynse Sep 7, 2023

Choose a reason for hiding this comment

sitio-couto Sep 7, 2023

Choose a reason for hiding this comment

gysit commented Sep 7, 2023

sitio-couto commented Sep 7, 2023

zero9178 left a comment

Choose a reason for hiding this comment

sitio-couto commented Sep 8, 2023

zero9178 commented Sep 8, 2023