SCF To Calyx Support Float Add and Float SeqMemory Read/Write #6953

jiahanxie353 · 2024-04-25T19:04:16Z

This is the second patch stacking on #6952

This patch tries to legalize AddFOp and support floating point SeqMemory read/write in SCFToCalyx.

mikeurbach · 2024-04-26T00:57:19Z

I'd like to hear more about what the story for floating point types is. There's a reason nothing in CIRCT handles them--hardware IRs work very naturally for bitvectors modeled as (signless) integer types. But floating point is a whole beast with very different hardware circuits. How does Calyx handle floats?

jiahanxie353 · 2024-04-26T14:22:46Z

I'd like to hear more about what the story for floating point types is. There's a reason nothing in CIRCT handles them--hardware IRs work very naturally for bitvectors modeled as (signless) integer types. But floating point is a whole beast with very different hardware circuits. How does Calyx handle floats?

Sure! Calyx plans to introduce floating point adders and multipliers as primitives using HardFloat

hardware IRs work very naturally for bitvectors modeled as (signless) integer types

Suppose Calyx can do floating point manipulations, then the crux lies in lowering Calyx to HW dialect. Can we convert the floating point number to the corresponding bitvector based on IEEE754 and the problem is then solved? Am I oversimplifying the prblem?

mikeurbach · 2024-04-26T16:30:47Z

Thanks for the pointer. Yes, I think the crux of the problem is how Calyx floating points convert to bitvectors in the HW dialect. I don't think anyone in CIRCT has modeled IEEE754 floats, but if you are fine modeling them as a bitvector of the appropriate width, the core CIRCT dialects should support that.

jiahanxie353 · 2024-04-26T17:23:38Z

Thanks for the pointer. Yes, I think the crux of the problem is how Calyx floating points convert to bitvectors in the HW dialect. I don't think anyone in CIRCT has modeled IEEE754 floats, but if you are fine modeling them as a bitvector of the appropriate width, the core CIRCT dialects should support that.

Sounds good! I'm super excited to try this out!

…memory to write/read floating point data

…ing point) op to pass in write_data

mikeurbach

This is looking good.

I would suggest to break it up into smaller changes if possible. It seems there are a few things that can be added in separate PRs:

Addition of the constant op
Addition of the floating point add op
Addition of the memory handling

Also, please include both happy path and failure tests, as applicable.

mikeurbach · 2024-05-02T16:05:52Z

lib/Dialect/Calyx/CalyxOps.cpp

@@ -2911,7 +2989,53 @@ LogicalResult SliceLibOp::verify() {
            DictionaryAttr::get(getContext())};                                \
  }

+#define ImplBinFloatingPointOpCellInterface(OpType)                            \


We generally discourage macros. Could this be templated on OpType instead?

+1, I see ImplBinPipeOpCellInterface is doing something similar and I'm not sure why
upon a quick glance. It might be OK to refactor both in a follow-up PR, or open an issue to do so and add a TODO.

mikeurbach · 2024-05-02T16:08:08Z

include/circt/Dialect/Calyx/CalyxPrimitives.td

+      AnySignlessInteger:$execptionalFlags, I1:$done);
+}
+
+def AddFNOp : ArithBinaryFloatingPointLibraryOp<"addFN"> {}


What is the addFN name? "Add floating number" or something like that? I don't usually nitpick on the mnemonics, but it is worth thinking through. I get the impression this aligns with Calyx native IR, so if that's the case, addFN sounds good to me.

cgyurgyik · 2024-05-05T00:16:06Z

include/circt/Dialect/Calyx/CalyxPrimitives.td

+    // Equivalent generic form
+    %1 = "calyx.constant"() {value = 42 : i32} : () -> i32


Nit: I don't this is necessary; it is just a detail of LLVM.

cgyurgyik · 2024-05-05T00:21:10Z

lib/Dialect/Calyx/CalyxOps.cpp

@@ -2911,7 +2989,53 @@ LogicalResult SliceLibOp::verify() {
            DictionaryAttr::get(getContext())};                                \
  }

+#define ImplBinFloatingPointOpCellInterface(OpType)                            \


+1, I see ImplBinPipeOpCellInterface is doing something similar and I'm not sure why
upon a quick glance. It might be OK to refactor both in a follow-up PR, or open an issue to do so and add a TODO.

cgyurgyik · 2024-05-05T00:23:03Z

lib/Dialect/Calyx/Export/CalyxEmitter.cpp

+  unsigned bitWidth =
+      cell.getOutputPorts()[0].getType().getIntOrFloatBitWidth();
+  unsigned expWidth, sigWidth;
+  if (bitWidth == 16) {


Shouldn't this be expWidth, sigWidth, and signBit?

cgyurgyik · 2024-05-05T00:24:24Z

lib/Dialect/Calyx/Export/CalyxEmitter.cpp

+void Emitter::emitLibraryFloatingPoint(Operation *op) {
+  auto cell = cast<CellInterface>(op);
+  unsigned bitWidth =
+      cell.getOutputPorts()[0].getType().getIntOrFloatBitWidth();


Can we assert this has length > 0?

cgyurgyik · 2024-05-05T00:24:59Z

lib/Dialect/Calyx/Export/CalyxEmitter.cpp

+  unsigned bitWidth =
+      cell.getOutputPorts()[0].getType().getIntOrFloatBitWidth();
+  unsigned expWidth, sigWidth;
+  if (bitWidth == 16) {


I know this is the default size for Half, but there are other 16-bit values. Maybe comment saying this is for Half-float?

cgyurgyik · 2024-05-05T00:28:17Z

lib/Dialect/Calyx/Transforms/CalyxLoweringUtils.cpp

+bool MemoryInterface::isSeqMem() {
+  if (auto *memOp = std::get_if<calyx::SeqMemoryOp>(&impl); memOp) {
+    return true;
+  }
+  return false;
+}
+


Suggested change

bool MemoryInterface::isSeqMem() {

if (auto *memOp = std::get_if<calyx::SeqMemoryOp>(&impl); memOp) {

return true;

}

return false;

}

bool MemoryInterface::isSeqMem() {

return std::holds_alternative<calyx::SeqMemoryOp>(&impl);

}

cgyurgyik · 2024-05-05T00:28:52Z

lib/Dialect/Calyx/Transforms/CalyxLoweringUtils.cpp

@@ -730,7 +737,8 @@ BuildBasicBlockRegs::partiallyLowerFuncToComp(mlir::func::FuncOp funcOp,

    for (auto arg : enumerate(block->getArguments())) {
      Type argType = arg.value().getType();
-      assert(isa<IntegerType>(argType) && "unsupported block argument type");
+      assert((isa<IntegerType>(argType) || isa<FloatType>(argType)) &&


Suggested change

assert((isa<IntegerType>(argType) || isa<FloatType>(argType)) &&

assert(isa<IntegerType, FloatType>(argType) &&

(I think this should work)

cgyurgyik · 2024-05-05T00:29:07Z

lib/Dialect/Calyx/Transforms/CalyxLoweringUtils.cpp

@@ -753,11 +761,11 @@ BuildReturnRegs::partiallyLowerFuncToComp(mlir::func::FuncOp funcOp,

  for (auto argType : enumerate(funcOp.getResultTypes())) {
    auto convArgType = calyx::convIndexType(rewriter, argType.value());
-    assert(isa<IntegerType>(convArgType) && "unsupported return type");
-    unsigned width = convArgType.getIntOrFloatBitWidth();
+    assert((isa<IntegerType>(convArgType) || isa<FloatType>(convArgType)) &&


(Similar case for here)

cgyurgyik · 2024-05-05T00:33:06Z

(Hi sorry, I'm just seeing this is in draft mode. Feel free to ignore my comments.)

jiahanxie353 force-pushed the scf-calyx-float branch from 54039cd to 22b3ddc Compare April 26, 2024 17:40

supports creating registers with floating point types

bc24c15

jiahanxie353 force-pushed the scf-calyx-float branch from 22b3ddc to 9daf0cf Compare May 2, 2024 03:10

legalize AddFOp for floating point addition; and supports sequential …

7f612d1

…memory to write/read floating point data

jiahanxie353 force-pushed the scf-calyx-float branch from 9daf0cf to 9552293 Compare May 2, 2024 03:13

jiahanxie353 added 2 commits May 1, 2024 23:23

lower calyx sequential memory to hw; have a new calyx constant (float…

0051651

…ing point) op to pass in write_data

add addFN to Calyx

3c375e3

jiahanxie353 force-pushed the scf-calyx-float branch from 9552293 to 3c375e3 Compare May 2, 2024 03:27

mikeurbach reviewed May 2, 2024

View reviewed changes

cgyurgyik reviewed May 5, 2024

View reviewed changes

jiahanxie353 mentioned this pull request May 23, 2024

Calyx ConstantOp Support #7086

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCF To Calyx Support Float Add and Float SeqMemory Read/Write #6953

SCF To Calyx Support Float Add and Float SeqMemory Read/Write #6953

jiahanxie353 commented Apr 25, 2024

mikeurbach commented Apr 26, 2024

jiahanxie353 commented Apr 26, 2024 •

edited

mikeurbach commented Apr 26, 2024

jiahanxie353 commented Apr 26, 2024

mikeurbach left a comment

mikeurbach May 2, 2024

cgyurgyik May 5, 2024

mikeurbach May 2, 2024

cgyurgyik May 5, 2024

cgyurgyik May 5, 2024

cgyurgyik May 5, 2024

cgyurgyik May 5, 2024

cgyurgyik May 5, 2024

cgyurgyik May 5, 2024

cgyurgyik May 5, 2024

cgyurgyik May 5, 2024

cgyurgyik commented May 5, 2024

		// Equivalent generic form
		%1 = "calyx.constant"() {value = 42 : i32} : () -> i32

	assert((isa<IntegerType>(argType) \|\| isa<FloatType>(argType)) &&
	assert(isa<IntegerType, FloatType>(argType) &&

SCF To Calyx Support Float Add and Float SeqMemory Read/Write #6953

Are you sure you want to change the base?

SCF To Calyx Support Float Add and Float SeqMemory Read/Write #6953

Conversation

jiahanxie353 commented Apr 25, 2024

mikeurbach commented Apr 26, 2024

jiahanxie353 commented Apr 26, 2024 • edited

mikeurbach commented Apr 26, 2024

jiahanxie353 commented Apr 26, 2024

mikeurbach 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

cgyurgyik commented May 5, 2024

jiahanxie353 commented Apr 26, 2024 •

edited