[Calyx] Binary Floating Point MulF Operator #7769

jiahanxie353 · 2024-11-03T15:39:18Z

Add support for floating point multiplication in Calyx

rachitnigam · 2024-11-03T16:46:25Z

Okay, I think we should move the floating point operators into their own separate definition because they are so much more complicated. Can we get the syntax: calyx.ieee754.add and calyx.ieee754.mul working instead of adding the floating-point operations directly into the same namespace as everything else?

include/circt/Dialect/Calyx/CalyxPrimitives.td

test/Dialect/Calyx/emit.mlir

test/Conversion/SCFToCalyx/convert_simple.mlir

test/Dialect/Calyx/emit.mlir

jiahanxie353 · 2024-11-06T03:12:25Z

marking ready for review again!

test/Dialect/Calyx/emit.mlir

include/circt/Dialect/Calyx/CalyxPrimitives.td

jiahanxie353 · 2024-11-06T04:32:19Z

test/Dialect/Calyx/emit.mlir

+
+module attributes {calyx.entrypoint = "main"} {
+  // CHECK: import "primitives/float/mulFN.futil";
+  calyx.component @main(%in0: i32, %clk: i1 {clk}, %reset: i1 {reset}, %go: i1 {go}) -> (%out0: i32, %done: i1 {done}) {


Please add a CHECK-NEXT: for this line.

wait it is added here right? I believe you are looking for std_mulFN_0 = std_mulFN(8, 24, 32)?

Ah, can this not use CHECK-NEXT since we know exactly which line produces the primitive?

you mean we don't need to check std_mulFN_0 = std_mulFN(8, 24, 32) at all?

No, can we ensure that the code is generated precisely as a consequence of that one line?

Sounds good, I'm updating to CHECK-DAG to ensure your request of checking the code is generated by the correct line sequentially; as well as avoiding checking some boilerplate code that will be otherwise generated by CHECK-NEXT. Please let me know if it makes sense.

cgyurgyik

A few smaller comments.

cgyurgyik · 2024-11-13T20:50:50Z

include/circt/Dialect/Calyx/CalyxLoweringUtils.h

@@ -424,6 +424,10 @@ class ComponentLoweringStateInterface {
    Block *body = component.getBodyBlock();
    builder.setInsertionPoint(body, body->begin());
    auto name = TLibraryOp::getOperationName().split(".").second;
+    if (std::is_same<calyx::AddFNOp, TLibraryOp>::value)
+      name = "std_addFN";


Nit: isn't the better way to do this is remove the .ieee754 prefix for any F operations?

The entire prefix + name is calyx.ieee754.add/calyx.ieee754.mul, removing ieee754 will simply become add/mul; but we want std_addFN/std_mulFN after emitting to Calyx native compiler.

More concretely, I think there is a better way to do this than manually write a new if statement for each floating point operation. It'd be nice to avoid this every time:

if (std::is_same<calyx::<MyNewFloatingPointOp, TLibraryOP>::value) name = "std_MyNewFloatingPointOp";

Ideally, since we're already distinguishing them without a _std prefix, we could infer this is some floating point operation, and then check what standard it conforms too.

/// Returns the technical floating point this operation conforms to, e.g., "ieee754" /// (This should probably be an enum, which is subsequently converted to a string.) std::string standard() { ... } if (auto fOp = dyn_cast<FloatingPointOp>(op)): name.remove(llvm::join(".", fOp.standard())); name.prepend("std_");

(I'm not saying you should spend a lot of time doing this, it is a mere suggestion. In general, I do think the conforming technical standard of a floating point operation should be a field of the op so it can referenced during pattern rewrites.)

I've thought about this, I don't think it's worth it now.
In the foreseeable future, only IEEE754 floating point is integrated; and moreover, only Add and Mul are present in the Berkeley HardFloat.
My opinion is we can address future needs and refactor the code when there is an actual, concrete need.
But I totally get what you are saying.

Sorry, let me take it back.
When doing some research on the floating point comparison issue #7821, I found Berkeley HardFloat has division, square root, and comparisons as well.
So I definitely need to use a better design to incorporate all of these calyx::FloatingPointOps, as you suggested.

cgyurgyik · 2024-11-13T20:51:33Z

lib/Dialect/Calyx/Export/CalyxEmitter.cpp

@@ -1019,11 +1024,15 @@ void Emitter::emitLibraryFloatingPoint(Operation *op) {
    return;
  }

-  StringRef opName = op->getName().getStringRef();
+  std::string opName;
+  if (isa<calyx::AddFNOp>(op))


Still not a fan of this if-else, it implies that there will only ever exist two FN functions, add and mul.

jiahanxie353 · 2024-11-19T02:36:15Z

I'm using FloatingPoint trait to check if an operation deals with floating point values; and use an enum to distinguish between different standards.

May not be the best design, happy to hear folks' opinions.

cgyurgyik

Nice, definitely a big improvement. A few more comments.

cgyurgyik · 2024-11-19T16:24:40Z

include/circt/Dialect/Calyx/CalyxLoweringUtils.h

+        std::string modified = llvm::join_items("", "std_", name, "FN");
+        name = modified;


To avoid a potential copy, just use the r-value:

Suggested change

std::string modified = llvm::join_items("", "std_", name, "FN");

name = modified;

name = llvm::join_items("", "std_", name, "FN");

I don't think name = llvm::join_items("", "std_", name, "FN"); work
llvm::join_items returns a temporary std::string so it cannot directly assign to a StringRef (name here). So I had to assign it to a variable modified before assigning to name...

Is there any idiomatic way to deal with this, name = StringRef(llvm::join_items("", "std_", name, "FN"))?

Oh then isn't this a dangling reference since modified is destroyed when we exit the if statement? I think the safest course of action is to make name a std::string.

include/circt/Dialect/Calyx/CalyxLoweringUtils.h

cgyurgyik · 2024-11-19T16:27:06Z

include/circt/Dialect/Calyx/CalyxLoweringUtils.h

+      auto standard = TLibraryOp::getFloatingPointStandard();
+      if (standard == FloatingPointStandard::IEEE754) {


I think a switch would be appropriate here, so that that if someone adds another case the compiler will complain if we don't update this.

Suggested change

auto standard = TLibraryOp::getFloatingPointStandard();

if (standard == FloatingPointStandard::IEEE754) {

switch(TLibraryOp::getFloatingPointStandard()) {

case FloatingPointStandard::IEEE754:

...

}

cgyurgyik · 2024-11-19T16:29:51Z

lib/Dialect/Calyx/Export/CalyxEmitter.cpp

@@ -1019,11 +1024,15 @@ void Emitter::emitLibraryFloatingPoint(Operation *op) {
    return;
  }

-  StringRef opName = op->getName().getStringRef();
+  std::string opName;
+  if (isa<calyx::AddFNOp>(op))


Still not a fan of this if-else, it implies that there will only ever exist two FN functions, add and mul.

cgyurgyik · 2024-11-19T16:32:32Z

include/circt/Dialect/Calyx/CalyxOps.h

+  IEEE754,
+};
+
+/// Signals that the following operation operates on floating point values.


Suggested change

/// Signals that the following operation operates on floating point values.

/// Signals that the following operation operates on floating point values.

/// Unlike other CIRCT dialects, Calyx treats inputs as signless values and

/// leaves their interpretation to the respective operation.

cgyurgyik · 2024-11-19T16:34:16Z

include/circt/Dialect/Calyx/CalyxPrimitives.td


-def AddFNOp : ArithBinaryFloatingPointLibraryOp<"addFN"> {
+def AddFNOp : ArithBinaryFloatingPointLibraryOp<"ieee754.add"> {


This is a big improvement. IMO, a better name for this might be: AddFOpIeee754 and similar for below. I imagine future ops would follow similar patterns, e.g., AddFOpBf16.

jiahanxie353 · 2024-11-19T23:22:06Z

I have fixed the StringRef vs std::string for name problem;
and have a re-design for distinguish if it's a floating point operation using FloatingPointInterface and SFINAE, so that getting the standard and the Calyx library name is more elegant.

cgyurgyik · 2024-11-19T23:26:31Z

include/circt/Dialect/Calyx/CalyxLoweringUtils.h

+    if constexpr (HasGetFloatingPointStandard<TLibraryOp>::value) {
+      switch (TLibraryOp::getFloatingPointStandard()) {
+      case FloatingPointStandard::IEEE754: {
+        std::string prefix = "ieee754.";


Suggested change

std::string prefix = "ieee754.";

constexpr char[] prefix = "ieee754.";

cgyurgyik · 2024-11-19T23:27:11Z

include/circt/Dialect/Calyx/CalyxLoweringUtils.h

+      case FloatingPointStandard::IEEE754: {
+        std::string prefix = "ieee754.";
+        assert(name.find(prefix) == 0 &&
+               "IEEE754 type operation's name must begin with 'ieee754'");


Code duplication nit: use prefix instead of repeating the string ieee754.

cgyurgyik · 2024-11-19T23:43:31Z

include/circt/Dialect/Calyx/CalyxLoweringUtils.h

@@ -417,13 +417,37 @@ class ComponentLoweringStateInterface {
    }
  }

+  template <typename T, typename = void>
+  struct HasGetFloatingPointStandard : std::false_type {};


Suggestion: this name is a bit confusing, maybe just IsFloatingPoint?

…naming for mulf as well as addf to be mulFN/addFN

…h different floating point standards more programmably

…Instance

jiahanxie353 · 2024-11-20T02:46:36Z

thanks for the suggestions @cgyurgyik ! I've also changed AddFNOp/MulFNOp to better naming standards

cgyurgyik

This looks good to me. I believe Rachit is on vacation, so I think it would be ok to merge (when CI passes) if this is blocking you, and then address any comments later.

jiahanxie353 · 2024-11-20T04:00:16Z

This looks good to me. I believe Rachit is on vacation, so I think it would be ok to merge (when CI passes) if this is blocking you, and then address any comments later.

Sounds good, thank you!

jiahanxie353 requested review from mikeurbach, mortbopet, rachitnigam, cgyurgyik and andrewb1999 as code owners November 3, 2024 15:39

jiahanxie353 commented Nov 3, 2024

View reviewed changes

include/circt/Dialect/Calyx/CalyxPrimitives.td Outdated Show resolved Hide resolved

jiahanxie353 commented Nov 3, 2024

View reviewed changes

test/Dialect/Calyx/emit.mlir Outdated Show resolved Hide resolved

jiahanxie353 force-pushed the float-mul branch from 8bda83f to f54ffee Compare November 3, 2024 20:09

jiahanxie353 commented Nov 3, 2024

View reviewed changes

test/Conversion/SCFToCalyx/convert_simple.mlir Outdated Show resolved Hide resolved

jiahanxie353 commented Nov 3, 2024

View reviewed changes

test/Dialect/Calyx/emit.mlir Outdated Show resolved Hide resolved

rachitnigam reviewed Nov 3, 2024

View reviewed changes

test/Dialect/Calyx/emit.mlir Outdated Show resolved Hide resolved

jiahanxie353 mentioned this pull request Nov 4, 2024

[Calyx] constant op #7770

Merged

rachitnigam added the Calyx The Calyx dialect label Nov 5, 2024

jiahanxie353 force-pushed the float-mul branch from f54ffee to 8a270d7 Compare November 6, 2024 03:08

rachitnigam reviewed Nov 6, 2024

View reviewed changes

test/Dialect/Calyx/emit.mlir Outdated Show resolved Hide resolved

rachitnigam reviewed Nov 6, 2024

View reviewed changes

include/circt/Dialect/Calyx/CalyxPrimitives.td Outdated Show resolved Hide resolved

jiahanxie353 commented Nov 6, 2024

View reviewed changes

jiahanxie353 force-pushed the float-mul branch from 5a4c97c to 6a6d127 Compare November 6, 2024 21:04

cgyurgyik reviewed Nov 13, 2024

View reviewed changes

jiahanxie353 force-pushed the float-mul branch 2 times, most recently from 5e94ef3 to 36de962 Compare November 19, 2024 02:34

cgyurgyik reviewed Nov 19, 2024

View reviewed changes

jiahanxie353 added 3 commits November 19, 2024 21:38

support mulf op

4c691df

wrap floating point mul namespace with calyx.ieee754

073c5d0

making mulf to use integer type as inputs and result; and change the …

ace9ff4

…naming for mulf as well as addf to be mulFN/addFN

jiahanxie353 added 8 commits November 19, 2024 21:38

remove 'std_' before ieee754 operations

5ecc93f

remove std_mulFN declaration in emit test

06d69fc

use CHECK DAG to ensure the sequential checking of each line

c861cbf

add floating point trait for floating point operators for dealing wit…

8503f2b

…h different floating point standards more programmably

introduce floating point interface and use sfinae for getNewLibraryOp…

76715a9

…Instance

small fix, remove redundant code

c3b75da

some fixes

ba895ba

change Add/MulFN to Add/MulFOpIEEE754 for better naming standard

1e557ba

jiahanxie353 force-pushed the float-mul branch from 1e8adca to 1e557ba Compare November 20, 2024 02:41

clang format

da5094b

cgyurgyik approved these changes Nov 20, 2024

View reviewed changes

jiahanxie353 merged commit b2aebf0 into llvm:main Nov 20, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Calyx] Binary Floating Point MulF Operator #7769

[Calyx] Binary Floating Point MulF Operator #7769

jiahanxie353 commented Nov 3, 2024

rachitnigam commented Nov 3, 2024

jiahanxie353 commented Nov 6, 2024

jiahanxie353 Nov 6, 2024

rachitnigam Nov 6, 2024

jiahanxie353 Nov 6, 2024

rachitnigam Nov 15, 2024

jiahanxie353 Nov 15, 2024

cgyurgyik left a comment

cgyurgyik Nov 13, 2024

jiahanxie353 Nov 13, 2024 •

edited

Loading

cgyurgyik Nov 13, 2024

jiahanxie353 Nov 14, 2024 •

edited

Loading

jiahanxie353 Nov 15, 2024

cgyurgyik Nov 13, 2024

cgyurgyik Nov 19, 2024

jiahanxie353 commented Nov 19, 2024

cgyurgyik left a comment

cgyurgyik Nov 19, 2024

jiahanxie353 Nov 19, 2024

cgyurgyik Nov 19, 2024

cgyurgyik Nov 19, 2024

cgyurgyik Nov 19, 2024

cgyurgyik Nov 19, 2024

cgyurgyik Nov 19, 2024 •

edited

Loading

jiahanxie353 commented Nov 19, 2024

cgyurgyik Nov 19, 2024

cgyurgyik Nov 19, 2024

cgyurgyik Nov 19, 2024

jiahanxie353 commented Nov 20, 2024

cgyurgyik left a comment

jiahanxie353 commented Nov 20, 2024

		std::string modified = llvm::join_items("", "std_", name, "FN");
		name = modified;

	std::string modified = llvm::join_items("", "std_", name, "FN");
	name = modified;
	name = llvm::join_items("", "std_", name, "FN");

		auto standard = TLibraryOp::getFloatingPointStandard();
		if (standard == FloatingPointStandard::IEEE754) {


		def AddFNOp : ArithBinaryFloatingPointLibraryOp<"addFN"> {
		def AddFNOp : ArithBinaryFloatingPointLibraryOp<"ieee754.add"> {

	std::string prefix = "ieee754.";
	constexpr char[] prefix = "ieee754.";

[Calyx] Binary Floating Point MulF Operator #7769

[Calyx] Binary Floating Point MulF Operator #7769

Conversation

jiahanxie353 commented Nov 3, 2024

rachitnigam commented Nov 3, 2024

jiahanxie353 commented Nov 6, 2024

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

cgyurgyik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiahanxie353 Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiahanxie353 Nov 14, 2024 • 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

jiahanxie353 commented Nov 19, 2024

cgyurgyik 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

cgyurgyik Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

jiahanxie353 commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiahanxie353 commented Nov 20, 2024

cgyurgyik left a comment

Choose a reason for hiding this comment

jiahanxie353 commented Nov 20, 2024

jiahanxie353 Nov 13, 2024 •

edited

Loading

jiahanxie353 Nov 14, 2024 •

edited

Loading

cgyurgyik Nov 19, 2024 •

edited

Loading