Page MenuHomePhabricator

Make kDynamicSize equal to kDynamicStrideOrOffset
ClosedPublic

Authored by khasanovaa on Sep 28 2022, 6:23 AM.

Diff Detail

Event Timeline

khasanovaa created this revision.Sep 28 2022, 6:23 AM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
khasanovaa requested review of this revision.Sep 28 2022, 6:23 AM
pifon2a edited the summary of this revision. (Show Details)Sep 28 2022, 6:34 AM

Thanks for removing this huge foot gun!

@Mogball would you have a suggestion on the best approach for the printing/parsing apparent regression here ?
The tricky bit is that this touches on the generic parser / printer..
It is possible we have to make tosa tests use proper custom syntax.

The fact that the magic number goes form -1 to -9223372036854775808 is an annoying detail that should not distract from the much bigger footgun (e.g. we've seen people actually perform arithmetic that "just works" with -1... ).

mlir/python/mlir/dialects/_linalg_ops_ext.py
42 ↗(On Diff #463527)

Let's avoid replacing. magic constant by another one, can we expose ShapedType::kDynamicSize to the C API and python?

mlir/test/Conversion/TosaToLinalg/tosa-to-linalg.mlir
545 ↗(On Diff #463527)

Could we improve printing / parsing everywhere the magic constant shifts ?

mlir/test/python/dialects/linalg/ops.py
66 ↗(On Diff #463527)

Let's avoid replacing. magic constant by another one, can we expose ShapedType::kDynamicSize to the C API and python? (here and below)

@kiranchandramohan this PR breaks flang. Could it be that there is a use of kDynamicSize instead of -1 somewhere?

@kiranchandramohan this PR breaks flang. Could it be that there is a use of kDynamicSize instead of -1 somewhere?

I don't think there is a direct use of kDynamicSize in the code. ShapedType and RankedTensorType are used in a few places. I see that a lot of tests are failing at different places and due to different reasons including crashes.

CC: @jeanPerier, @clementval

@kiranchandramohan this PR breaks flang. Could it be that there is a use of kDynamicSize instead of -1 somewhere?

I don't think there is a direct use of kDynamicSize in the code. ShapedType and RankedTensorType are used in a few places. I see that a lot of tests are failing at different places and due to different reasons including crashes.

CC: @jeanPerier, @clementval

Maybe coming from this -1 https://github.com/llvm/llvm-project/blob/01adf1f3e50a87e290cedb47fdf9d1e0cc635015/flang/include/flang/Optimizer/Dialect/FIRTypes.td#L474

ftynse added inline comments.Sep 29 2022, 3:44 AM
mlir/python/mlir/dialects/_linalg_ops_ext.py
42 ↗(On Diff #463527)

FYI, they are already available in both C and Python.

Replace magic constants with KDynamicSize in python

pifon2a accepted this revision.Oct 11 2022, 6:00 AM

@clementval @kiranchandramohan @jeanPerier This PR will land in the end of the week. Could you please make sure that

static constexpr Extent getUnknownExtent() { return -1; }

is updated?

This revision is now accepted and ready to land.Oct 11 2022, 6:00 AM

@clementval @kiranchandramohan @jeanPerier This PR will land in the end of the week. Could you please make sure that

static constexpr Extent getUnknownExtent() { return -1; }

is updated?

Why isn't that part of this patch?

@clementval @kiranchandramohan @jeanPerier This PR will land in the end of the week. Could you please make sure that

static constexpr Extent getUnknownExtent() { return -1; }

is updated?

Why isn't that part of this patch?

Because I do not know how to compile/test flang correctly. It should be smth like https://reviews.llvm.org/D135675. Maybe, it just works out-of-the-box.

@clementval @kiranchandramohan @jeanPerier This PR will land in the end of the week. Could you please make sure that

static constexpr Extent getUnknownExtent() { return -1; }

is updated?

Why isn't that part of this patch?

Because I do not know how to compile/test flang correctly. It should be smth like https://reviews.llvm.org/D135675. Maybe, it just works out-of-the-box.

@khasanovaa Can you rebase the patch so we can check if D135675 fixed the pre commit checks?

@clementval @kiranchandramohan @jeanPerier, it looks like my patch did not actually solve all problems. Could you please take a look? It is quite an important PR.

@clementval @kiranchandramohan @jeanPerier, it looks like my patch did not actually solve all problems. Could you please take a look? It is quite an important PR.

With the following changes, I see that all tests pass. But not sure whether the replacements have happened at all places.

diff --git a/flang/include/flang/Optimizer/Dialect/FIRTypes.td b/flang/include/flang/Optimizer/Dialect/FIRTypes.td
index f179071f1943..0d06e1d118ea 100644
--- a/flang/include/flang/Optimizer/Dialect/FIRTypes.td
+++ b/flang/include/flang/Optimizer/Dialect/FIRTypes.td
@@ -128,7 +128,7 @@ def fir_CharacterType : FIR_Type<"Character", "char"> {
     static constexpr LenType singleton() { return 1; }
 
     /// Character has a LEN value which is not a compile-time known constant.
-    static constexpr LenType unknownLen() { return -1; }
+    static constexpr LenType unknownLen() { return mlir::ShapedType::kDynamicSize; }
 
     /// Character LEN is a runtime value.
     bool hasDynamicLen() { return getLen() == unknownLen(); }
diff --git a/flang/lib/Optimizer/Transforms/AffinePromotion.cpp b/flang/lib/Optimizer/Transforms/AffinePromotion.cpp
index ae152cf3a524..92f38d73790e 100644
--- a/flang/lib/Optimizer/Transforms/AffinePromotion.cpp
+++ b/flang/lib/Optimizer/Transforms/AffinePromotion.cpp
@@ -410,7 +410,7 @@ createAffineOps(mlir::Value arrayRef, mlir::PatternRewriter &rewriter) {
   auto affineApply = rewriter.create<mlir::AffineApplyOp>(acoOp.getLoc(),
                                                           affineMap, indexArgs);
   auto arrayElementType = coordinateArrayElement(acoOp);
-  auto newType = mlir::MemRefType::get({-1}, arrayElementType);
+  auto newType = mlir::MemRefType::get({mlir::ShapedType::kDynamicSize}, arrayElementType);
   auto arrayConvert = rewriter.create<fir::ConvertOp>(acoOp.getLoc(), newType,
                                                       acoOp.getMemref());
   return std::make_pair(affineApply, arrayConvert);

[flang] Use ShapedType::kDynamicSize instead of -1

Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2022, 2:50 AM

@clementval @kiranchandramohan @jeanPerier, it looks like my patch did not actually solve all problems. Could you please take a look? It is quite an important PR.

With the following changes, I see that all tests pass. But not sure whether the replacements have happened at all places.

diff --git a/flang/include/flang/Optimizer/Dialect/FIRTypes.td b/flang/include/flang/Optimizer/Dialect/FIRTypes.td
index f179071f1943..0d06e1d118ea 100644
--- a/flang/include/flang/Optimizer/Dialect/FIRTypes.td
+++ b/flang/include/flang/Optimizer/Dialect/FIRTypes.td
@@ -128,7 +128,7 @@ def fir_CharacterType : FIR_Type<"Character", "char"> {
     static constexpr LenType singleton() { return 1; }
 
     /// Character has a LEN value which is not a compile-time known constant.
-    static constexpr LenType unknownLen() { return -1; }
+    static constexpr LenType unknownLen() { return mlir::ShapedType::kDynamicSize; }
 
     /// Character LEN is a runtime value.
     bool hasDynamicLen() { return getLen() == unknownLen(); }
diff --git a/flang/lib/Optimizer/Transforms/AffinePromotion.cpp b/flang/lib/Optimizer/Transforms/AffinePromotion.cpp
index ae152cf3a524..92f38d73790e 100644
--- a/flang/lib/Optimizer/Transforms/AffinePromotion.cpp
+++ b/flang/lib/Optimizer/Transforms/AffinePromotion.cpp
@@ -410,7 +410,7 @@ createAffineOps(mlir::Value arrayRef, mlir::PatternRewriter &rewriter) {
   auto affineApply = rewriter.create<mlir::AffineApplyOp>(acoOp.getLoc(),
                                                           affineMap, indexArgs);
   auto arrayElementType = coordinateArrayElement(acoOp);
-  auto newType = mlir::MemRefType::get({-1}, arrayElementType);
+  auto newType = mlir::MemRefType::get({mlir::ShapedType::kDynamicSize}, arrayElementType);
   auto arrayConvert = rewriter.create<fir::ConvertOp>(acoOp.getLoc(), newType,
                                                       acoOp.getMemref());
   return std::make_pair(affineApply, arrayConvert);

@kiranchandramohan I've added your patch. Now all tests pass, thank you!

We've submitted all changes that do not break tests (mostly -1 -> kDynamicSize).

pifon2a accepted this revision.Wed, Nov 16, 10:35 AM

Great, thanks for your diligence on finishing this important cleanup!

This revision was landed with ongoing or failed builds.Thu, Nov 17, 1:37 AM
This revision was automatically updated to reflect the committed changes.