Bit optim #1

yuxwang99 · 2023-05-04T13:09:31Z

Merge bits optimization passes into main

update new features implemented in StandardToHandshakeFPGA18

lucas-rami

Good job!

I've done a sort-of superficial review and will take a closer look at everything soon. In the meantime you can start addressing the comments that are already there. Most of them are about code style or about using the MLIR API to make your life easier :)

Also, please run clang-format (with default parameters) on all the files you have pushed. You just have to install the tool on your machine and then enable "format on save" in your IDE, so that your code is always properly formatted.

lucas-rami · 2023-05-07T21:29:28Z

.gitignore

The .gitignore file should not be changed.

lucas-rami · 2023-05-07T21:31:45Z

.vscode/c_cpp_properties.json

@@ -15,7 +15,7 @@
        "${workspaceFolder}/circt/llvm/build/tools/mlir/include/"
      ],
      "intelliSenseMode": "linux-clang-x64",
-      "compilerPath": "/usr/bin/clang",
+      "compilerPath": "/usr/local/bin/clang",


This file is just provided as an example project configuration for VSCode, so it should not be changed based on your local install.

lucas-rami · 2023-05-07T21:34:03Z

include/dynamatic/Transforms/BackwardUpdate.h

@@ -0,0 +1,19 @@
+//===- BackwardUpdate.h ---------*- C++ -*-===//


nit pick: follow the conventions from other file here and make the line 80 characters long
//===- <file_name> - <short description> ---*- C++ -*-===//

The same applies to all other files.

lucas-rami · 2023-05-07T21:35:57Z

include/dynamatic/Transforms/BackwardUpdate.h

+
+#include "dynamatic/Transforms/UtilsBitsUpdate.h"
+
+namespace backward {


Every namespace should be declared under the namespace dynamatic (so that they can be used as e.g., ::dynamatic::backward).

lucas-rami · 2023-05-07T21:41:37Z

include/dynamatic/Transforms/ForwardUpdate.h

+
+#include "dynamatic/Transforms/UtilsBitsUpdate.h"
+
+namespace forward {


Same comment as for the backward namespace: this should be declared under the dynamatic namespace.

Also, the number of public functions declared for the pass is small so I don't think we should have multiple namespaces for them (not even sure we should have a namespace below dynamatic...). Please merge backward and forward into one or simply use dynamatic everywhere.

Hi Lucas, different namespace is used because we define the same constructFuncMap function for forward, backward and validation(same functionality, different rules to revise the bit width). I think It might be easier for understanding, but since we pass different arguments for the same named function, I can also merge them into one.
Please let me know which one you think is better.

I think it is more in line with MLIR conventions to not have very small named namespaces. I think you should just remove all custom namespaces, and use the dynamatic namespace whenever you want to declare public functions. You can name your functions constructForwardFuncMap, constructBackwardFuncMap, etc., which makes it clear what each function is for.

lucas-rami · 2023-05-07T22:47:05Z

lib/Transforms/InitIndexType.cpp

@@ -0,0 +1,108 @@
+//===- InitIndexType.cpp -  Transform the Index Type to IntegerType with system bit width -------*- C++ -*-===//


nit: make this fit on 80 characters

lucas-rami · 2023-05-07T22:48:35Z

lib/Transforms/InitIndexType.cpp

+  OpBuilder builder(ctx);
+  SmallVector<Operation *> indexCastOps;
+
+  for (Operation &op : funcOp.getOps()){


nit: use auto for the type

lib/Transforms/UtilsBitsUpdate.cpp

lucas-rami · 2023-05-07T22:52:12Z

lib/Transforms/UtilsBitsUpdate.cpp

+        isa<handshake::ConditionalBranchOp>(*Op))
+        pass = true;
+
+    if (isa<mlir::arith::AddIOp>(*Op)  ||


Check for multiple types at once in a single isa call, like isa<Type1, Type2, ...>(op) instead of checking for each type individually.

lucas-rami · 2023-05-07T22:52:54Z

lib/Transforms/UtilsBitsUpdate.cpp

+
+  unsigned int opWidth;
+  if (isa<IndexType>(opVal.getType()))
+    opWidth = 64;


Use IndexType::kInternalStorageBitWidth

lucas-rami

Finally had time to completely review this. There are a lot of comments but the vast majority are trivial fixes. Once everything is addressed we'll be able to merge :) There are one or two things in UtilsBitsUpdate.cpp that I haven't looked at in details yet and I may still do a pass on them in the near future.

include/dynamatic/Transforms/UtilsBitsUpdate.h

lib/Transforms/InitCstWidth.cpp

lucas-rami · 2023-05-11T13:09:56Z

lib/Transforms/InitCstWidth.cpp

+      op->erase();
+    }
+  }
+  // llvm::errs() << "Constant saved bits " << savedBits << "\n";


Feel free to use LLVM_DEBUG here, for example

LLVM_DEBUG(llvm::dbgs() << "Number of saved bits is `" << savedBits << "\n");

lib/Transforms/InitCstWidth.cpp

lib/Transforms/UtilsBitsUpdate.cpp

lucas-rami

Couple details to address still, but we're super close!

lib/Transforms/InitCstWidth.cpp

lib/Transforms/InitIndexType.cpp

lib/Transforms/OptimizeBits.cpp

lib/Transforms/UtilsBitsUpdate.cpp

lucas-rami

I think my last comments cover everything. I also reopened two resolved conversations, but they're just about a formatting issue. After you address those and fix the merge conflict with main we can merge this :)

lib/Transforms/InitCstWidth.cpp

lib/Transforms/OptimizeBits.cpp

test/Transforms/optimize-bitwidth.mlir

yuxwang99 added 30 commits March 23, 2023 11:01

initialize for bit Optimize

b560c22

Merge remote-tracking branch 'origin/main' into bitOptim

f904ad8

update new features implemented in StandardToHandshakeFPGA18

merge new features from main

1249931

initialize file created for bits optimization

4c4a944

merge circt update

fb92e71

init bits optimization pass

4b966a2

Merge branch 'main' of github.com:EPFL-LAP/dynamatic into bitOptim

954f9d0

merge submodule circt from main

92e4321

Merge branch 'bitOptim' of github.com:EPFL-LAP/dynamatic into bitOptim

f5324eb

update bits optimize initialization

dad2289

attach circt to branch bitOptim

d320a05

Merge branch 'main' of github.com:EPFL-LAP/dynamatic into bitOptim

39872b4

infrastructure for forward pass

9f83eb9

methods to convert operation type

74bc628

add opname

869bac0

init op-function map

03416b0

init recursive settype

408e22c

conversion type for bits optimize

bfbb468

init recursive update pass for d_store

86e9260

delete patternconversion style implementation

c883258

prepare to pull circt

eb22554

merge circt update

2cb7920

register the arith dialect

25a7e6b

split index init and cst init pass

8d26980

add new features of bit width forward computation

746245a

update the utils header and source file

45d00a6

use function to set different update operation strategy

22afac9

update update op flag

4e9bec7

Merge branch 'main' of github.com:EPFL-LAP/dynamatic into bitOptim

14bda82

update mergecontrol

8ccbd06

lucas-rami reviewed May 7, 2023

View reviewed changes

yuxwang99 and others added 15 commits May 8, 2023 09:05

clang-formatting

ea509e2

merge forward backward update namespace into dynamatic

0a0b8dd

rename optimize bits pass

1c3ca05

config clang-format for header files

93b8db4

adapt utils for merge

575ccb0

apply clang-format on lib src files

bbf5c3e

merge main=theirs

b21198d

Update .gitignore

2922aba

Update .gitignore

207339d

Update .gitignore

cfc19e8

cancel output of bits saving

9eb4abe

Merge branch 'bitOptim' of github.com:EPFL-LAP/dynamatic into bitOptim

36aec81

cancel output style of bits saving

910b692

merge main

6f5e707

change handshake selectOp to arith selectOp

48d0817

lucas-rami reviewed May 11, 2023

View reviewed changes

yuxwang99 added 4 commits May 12, 2023 08:29

edit format

dfa6481

use llvm::typeswitch to set flags

ec01912

simplify replaceWithPredecessor implementation

45d2911

update vairable name after camelCase

39c4f08

lucas-rami reviewed May 16, 2023

View reviewed changes

change to llvm_debug info

3d139f7

lucas-rami reviewed May 25, 2023

View reviewed changes

lib/Transforms/InitCstWidth.cpp Outdated Show resolved Hide resolved

lib/Transforms/OptimizeBits.cpp Show resolved Hide resolved

lib/Transforms/OptimizeBits.cpp Show resolved Hide resolved

test/Transforms/optimize-bitwidth.mlir Show resolved Hide resolved

yuxwang99 and others added 3 commits May 30, 2023 13:40

define debug type

121d633

fix generate .mlir file with new PR

091e76d

Merge branch 'main' into bitOptim

1660931

lucas-rami merged commit abd3f1a into main May 30, 2023

albertfares mentioned this pull request Oct 13, 2023

[visual-dataflow] DOT/CSV Parsing #40

Merged

PigBrainOverflow mentioned this pull request Oct 9, 2024

[MLIR] [LLVM] ninja check-dynamatic failed #164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bit optim #1

Bit optim #1

yuxwang99 commented May 4, 2023

lucas-rami left a comment •

edited

Loading

lucas-rami May 7, 2023

lucas-rami May 7, 2023

lucas-rami May 7, 2023

lucas-rami May 7, 2023

lucas-rami May 7, 2023

yuxwang99 May 8, 2023

lucas-rami May 8, 2023

lucas-rami May 7, 2023

lucas-rami May 7, 2023

lucas-rami May 7, 2023

lucas-rami May 7, 2023

lucas-rami left a comment

lucas-rami May 11, 2023

lucas-rami left a comment

lucas-rami left a comment

		@@ -0,0 +1,19 @@
		//===- BackwardUpdate.h ---------- C++ --===//


		#include "dynamatic/Transforms/UtilsBitsUpdate.h"

		namespace backward {


		#include "dynamatic/Transforms/UtilsBitsUpdate.h"

		namespace forward {

		@@ -0,0 +1,108 @@
		//===- InitIndexType.cpp - Transform the Index Type to IntegerType with system bit width -------- C++ --===//

Bit optim #1

Bit optim #1

Conversation

yuxwang99 commented May 4, 2023

lucas-rami left a comment • edited Loading

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

lucas-rami left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucas-rami left a comment

Choose a reason for hiding this comment

lucas-rami left a comment

Choose a reason for hiding this comment

lucas-rami left a comment •

edited

Loading