Fix the deallocation problem for sequence op #1673

chentong319 · 2022-09-06T00:41:27Z

Since the sequence op is implemented as store pointers of element in memref, the buffer deallocation pass, which assumes no pointer, can not work correctly with previous sequence op lowering.
In this PR, the load or store of the pointer of sequence element is encapsulated in krnl sequence ops. These Ops are lowered after bufferization::deallocation pass. These krnl sequence ops defines the interface for bufferazation so that deallocation can be generated correctly.
A lit test to check deallocation result is added.

Signed-off-by: chentong319 <[email protected]>

AlexandreEichenberger · 2022-09-06T15:22:38Z

@chentong319 ready for reviewing?

AlexandreEichenberger

Trying to understand the scheme, which looks solid, but left me with a few questions. Will review this until I have a better overall understanding.

AlexandreEichenberger · 2022-09-06T20:45:51Z

src/Dialect/Krnl/Krnl.td

  }];
  let arguments = (ins AnyMemRef:$seq,
-                       Index:$index);
+                       Index:$index,
+                       DefaultValuedAttr<SI64Attr, "0">:$copy);


copy feels like a boolean attribute, see

DefaultValuedAttr<BoolAttr, "true">:$simdize,

for example

AlexandreEichenberger · 2022-09-06T20:50:09Z

src/Dialect/Krnl/Krnl.td

+    When the attribute 'copy' is 0: the extracted element is directly returned
+    When the attribute 'copy' is 1: the extracted element is copied and then return. 
+    The returned element is regarded as allocated by this Op because the 
+    element is created (copied) when inserted into the sequence.


sentence above is hard to parse.

AlexandreEichenberger · 2022-09-06T20:52:54Z

src/Dialect/Krnl/Krnl.td

+    When the attribute 'copy' is 1: the extracted element is copied and then return. 
+    The returned element is regarded as allocated by this Op because the 
+    element is created (copied) when inserted into the sequence.
+    Attribute 'copy' can be 0 when analysis or code generation guarantees 


I would expand a bit more on this. "If an optimization pass can determine that each element is only extracted once, then the copy=false can be used"

Is it required that each element be extracted once? If one is never extracted, should it still work? If so, "each element is extracted at most once"

every element is extracted exactly once

Technically, if an element is not used out of a list, why would it disable the copy=0. IMO, the correct statement is "at most once"

The element needs to be extracted or erased exactly once from the sequence. Otherwise, the memory for the element will be leaked.

If an element is not used, you imply that copy cannot be zero which makes no sense either.

Additionally, if a value is used exactly twice, would it make sense to have the first use with copy=1 but the second/last use with copy=0?

If an element is not used, you imply that copy cannot be zero which makes no sense either.

An element can be accessed only through KrnlSeqExtract. If there is no KrnlSeqExtract, there is no copy attribute.

src/Dialect/Krnl/Krnl.td

Signed-off-by: chentong319 <[email protected]>

AlexandreEichenberger

much better, suggested you use a method in our krnl create class, otherwise its all good after a spell check

AlexandreEichenberger · 2022-09-14T14:30:01Z

src/Conversion/ONNXToKrnl/ControlFlow/Loop.cpp

@@ -311,8 +311,9 @@ struct ONNXLoopOpLowering : public ConversionPattern {
                rewriter.setInsertionPointToStart(
                    &regionOp.bodyRegion().front());
                Value origIV = loopInd[0];
-                auto src = rewriter.create<KrnlSeqExtractOp>(
-                    loc, seqElementType, output, origIV);
+                auto src = rewriter.create<KrnlSeqExtractOp>(loc,


you could add a createKrnl method to generate a KrnlSeqExtract op where you can then skip the loc, and could accept an int for the attribute (and hide the IntegerAttr::get(rewriter.getIntegerType(1, false), 0)) inside that method.

just make the code a bit cleaner, obviously not mandatory but it would be nice.

AlexandreEichenberger · 2022-09-14T14:31:11Z

src/Conversion/ONNXToKrnl/ConvertONNXToKrnl.cpp

@@ -347,8 +347,9 @@ void FrontendToKrnlLoweringPass::runOnOperation() {

  // If `emitDealloc` is turned off, make sure we don't have buffer deallocation
  // at this level. Will use MLIR buffer-deallocation for this purpose instead.
-  if (!emitDealloc)
-    target.addIllegalOp<mlir::memref::DeallocOp>();
+  // However, since the SequenceErase needs to emit memref dealloc, the previous


Not for now, but I think we are pretty sure by now not to use the old buffer scheme. At sometimes, we could do a cleanup of the code, and eliminating all that is not needed anymore. Definitely not for this PR.

This PR needs to comment out the code because dealloc will be generated by SequenceErase.

AlexandreEichenberger · 2022-09-14T14:33:02Z

src/Conversion/ONNXToKrnl/Sequence/SequenceAt.cpp

+    IndexExpr fixedPosition = positionIE + boundIE;
+    positionIE = IndexExpr::select(condIE, fixedPosition, positionIE);
+
+    Value outputVal = rewriter.create<KrnlSeqExtractOp>(loc, outputMemRefType,


if you created a method to add this op, use it here too with create.krnl

Not a big fan of the dialect builder, especially when there is nothing abstracted out by builder.

Well we can abstract the handling of the int attribute, which I personally don't like to remember this awkward sequence... but if you don't want to do it, it is fine.

AlexandreEichenberger · 2022-09-14T14:33:44Z

src/Conversion/ONNXToKrnl/Sequence/SequenceErase.cpp

@@ -66,7 +66,7 @@ struct ONNXSequenceEraseOpLowering : public ConversionPattern {
      positionIE = IndexExpr::select(positionIE < 0, correctionIE, positionIE);
    }

-    // Copy before the insert
+    // Copy the elements before the position


great usage of consistent language, big thanks

AlexandreEichenberger · 2022-09-14T14:35:07Z

src/Conversion/SeqToMemref/KrnlSeqExtract.cpp

+      rewriter.replaceOp(op, output);
+      return success();
+    } else {
+      if (!output.getType().isa<MemRefType>())


this feels like an assert :-)

In future, we will handle the non MemRefType case. I usually use unreachable for that purpose: not a semantic error but an implementation limitation. Not sure about the convention in llvm.

AlexandreEichenberger · 2022-09-14T14:36:49Z

src/Conversion/SeqToMemref/KrnlSeqInsert.cpp

+    // is needed.
+
+    // Copy elements before the insertion position
+    rewriter.create<scf::ForOp>(loc, create.math.constantIndex(0),


Can you just add in the comment above why we use scf for instead of affine?
To me, it feels like it could be an affine as it looks pretty regular; is it called after lowering of affine to scf?
A small justification would explain your choice.

This is due to the limitation of our current driver: lowering affine is before deallocation pass. I will add a comment.

AlexandreEichenberger · 2022-09-14T14:39:45Z

src/Conversion/SeqToMemref/KrnlSeqInsert.cpp

+      : ConversionPattern(
+            typeConverter, KrnlSeqInsertOp::getOperationName(), 1, context) {}
+
+  LogicalResult matchAndRewrite(Operation *op, ArrayRef<Value> operands,


It would be nice if you could just explain in one sentence what the code pattern you generate is.

it seems to be:
Take the original list, grow the list by one element, copy all the elements before the insertion point, add the new one, and then copy all the elements after the insertion point.

AlexandreEichenberger · 2022-09-14T14:46:43Z

src/Dialect/Krnl/Krnl.td

+// a sequence. 'copy' here means to allocate a new memref and copy the content 
+// of the memref. The copy is a costly op to maintain the SSA requirement.
+// To reduce the overhead of copy, elements are not copied when
+// a new sequence is created from an exist sequence. Hower, when an element


Hower spelling. FYI, the "Code Spell Checker" from "Street Side Software" extension in VSCode works pretty well for me. It knows how to detect words in variables, comments,... I find it useful.

Thanks. I will try them.

AlexandreEichenberger

LGTM pending spelling check

Signed-off-by: chentong319 <[email protected]>

jenkins-droid · 2022-09-15T18:11:00Z

Jenkins Linux ppc64le Build #6653 [push] Fix the deallocation pro... started at 14:12

jenkins-droid · 2022-09-15T18:11:01Z

Jenkins Linux amd64 Build #7588 [push] Fix the deallocation pro... started at 13:10

jenkins-droid · 2022-09-15T18:11:03Z

Jenkins Linux s390x Build #7604 [push] Fix the deallocation pro... started at 14:11

jenkins-droid · 2022-09-15T19:20:28Z

Jenkins Linux amd64 Build #7588 [push] Fix the deallocation pro... passed after 1 hr 9 min

jenkins-droid · 2022-09-15T19:44:29Z

Jenkins Linux ppc64le Build #6653 [push] Fix the deallocation pro... passed after 1 hr 33 min

jenkins-droid · 2022-09-15T19:48:44Z

Jenkins Linux s390x Build #7604 [push] Fix the deallocation pro... passed after 1 hr 37 min

* compiled and run Signed-off-by: chentong319 <[email protected]> * restore CompilerPasses.cpp * handle copy Signed-off-by: chentong319 <[email protected]> * use attr Signed-off-by: chentong319 <[email protected]> * lit test Signed-off-by: chentong319 <[email protected]> * dealloc test Signed-off-by: chentong319 <[email protected]> * doc Signed-off-by: chentong319 <[email protected]> * format Signed-off-by: chentong319 <[email protected]> * ordering Signed-off-by: chentong319 <[email protected]> * format Signed-off-by: chentong319 <[email protected]> * change attr Signed-off-by: chentong319 <[email protected]> * doc Signed-off-by: chentong319 <[email protected]> * fix erase Signed-off-by: chentong319 <[email protected]> * doc Signed-off-by: chentong319 <[email protected]> * format Signed-off-by: chentong319 <[email protected]> * comment Signed-off-by: chentong319 <[email protected]> * response Signed-off-by: chentong319 <[email protected]> Signed-off-by: chentong319 <[email protected]> Signed-off-by: Stella Stamenova <[email protected]>

chentong319 added 15 commits August 30, 2022 15:42

compiled and run

67af465

Signed-off-by: chentong319 <[email protected]>

Merge remote-tracking branch 'upstream/main' into seq-krnl-2

552bf40

restore CompilerPasses.cpp

59a839f

Merge remote-tracking branch 'upstream/main' into seq-krnl-2

a545c8a

handle copy

74cf004

Signed-off-by: chentong319 <[email protected]>

Merge remote-tracking branch 'upstream/main' into seq-krnl-2

d339d14

use attr

18b13d0

Signed-off-by: chentong319 <[email protected]>

lit test

6b9b669

Signed-off-by: chentong319 <[email protected]>

dealloc test

d84db59

Signed-off-by: chentong319 <[email protected]>

doc

5762103

Signed-off-by: chentong319 <[email protected]>

Merge remote-tracking branch 'upstream/main' into seq-krnl-2

c655be5

format

a4c39b1

Signed-off-by: chentong319 <[email protected]>

ordering

6ae4b52

Signed-off-by: chentong319 <[email protected]>

format

5c4f172

Signed-off-by: chentong319 <[email protected]>

Merge remote-tracking branch 'upstream/main' into seq-krnl-2

0209994

chentong319 added the Ready for Review label Sep 6, 2022

chentong319 requested a review from AlexandreEichenberger September 6, 2022 19:49

AlexandreEichenberger reviewed Sep 6, 2022

View reviewed changes

chentong319 added 10 commits September 7, 2022 13:39

change attr

ba6ab93

Signed-off-by: chentong319 <[email protected]>

Merge remote-tracking branch 'upstream/main' into seq-krnl-2

234dc33

doc

3f41fdc

Signed-off-by: chentong319 <[email protected]>

Merge remote-tracking branch 'upstream/main' into seq-krnl-2

890cacb

Merge remote-tracking branch 'upstream/main' into seq-krnl-2

3025192

fix erase

1151d09

Signed-off-by: chentong319 <[email protected]>

doc

709e8be

Signed-off-by: chentong319 <[email protected]>

Merge remote-tracking branch 'upstream/main' into seq-krnl-2

afd03e5

format

61583e8

Signed-off-by: chentong319 <[email protected]>

comment

2375ff0

Signed-off-by: chentong319 <[email protected]>

AlexandreEichenberger reviewed Sep 14, 2022

View reviewed changes

AlexandreEichenberger approved these changes Sep 14, 2022

View reviewed changes

chentong319 added 2 commits September 15, 2022 12:25

response

f1189a0

Signed-off-by: chentong319 <[email protected]>

Merge remote-tracking branch 'upstream/main' into seq-krnl-2

ba8fb36

chentong319 merged commit c438b12 into onnx:main Sep 15, 2022

chentong319 deleted the seq-krnl-2 branch September 15, 2022 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the deallocation problem for sequence op #1673

Fix the deallocation problem for sequence op #1673

chentong319 commented Sep 6, 2022

AlexandreEichenberger commented Sep 6, 2022

AlexandreEichenberger left a comment

AlexandreEichenberger Sep 6, 2022

AlexandreEichenberger Sep 6, 2022

AlexandreEichenberger Sep 6, 2022

chentong319 Sep 8, 2022

AlexandreEichenberger Sep 12, 2022

chentong319 Sep 13, 2022

AlexandreEichenberger Sep 13, 2022

AlexandreEichenberger Sep 13, 2022

chentong319 Sep 13, 2022

chentong319 Sep 13, 2022

AlexandreEichenberger left a comment

AlexandreEichenberger Sep 14, 2022

AlexandreEichenberger Sep 14, 2022

chentong319 Sep 14, 2022

AlexandreEichenberger Sep 14, 2022

chentong319 Sep 14, 2022

AlexandreEichenberger Sep 14, 2022

AlexandreEichenberger Sep 14, 2022

AlexandreEichenberger Sep 14, 2022

chentong319 Sep 14, 2022

AlexandreEichenberger Sep 14, 2022

chentong319 Sep 14, 2022

AlexandreEichenberger Sep 14, 2022

AlexandreEichenberger Sep 14, 2022

chentong319 Sep 14, 2022

AlexandreEichenberger left a comment

jenkins-droid commented Sep 15, 2022

jenkins-droid commented Sep 15, 2022

jenkins-droid commented Sep 15, 2022

jenkins-droid commented Sep 15, 2022

jenkins-droid commented Sep 15, 2022

jenkins-droid commented Sep 15, 2022

Fix the deallocation problem for sequence op #1673

Fix the deallocation problem for sequence op #1673

Conversation

chentong319 commented Sep 6, 2022

AlexandreEichenberger commented Sep 6, 2022

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

jenkins-droid commented Sep 15, 2022

jenkins-droid commented Sep 15, 2022

jenkins-droid commented Sep 15, 2022

jenkins-droid commented Sep 15, 2022

jenkins-droid commented Sep 15, 2022

jenkins-droid commented Sep 15, 2022