dialects: (riscv) label region is multi block #1172

superlopuh · 2023-06-21T13:58:53Z

I put the return back in the middle because a lot more instructions are about to become terminators so it makes sense for them to be in order again

compor · 2023-06-21T14:10:41Z

Maybe move each terminator to a separate module in the filecheck files? I wanted to do just that in the terminator PRs but I forgot.

superlopuh · 2023-06-21T14:18:15Z

What for?

compor · 2023-06-21T14:27:22Z

What for?

Because I don't think it will pass the terminator tests in the middle of a module.
They need to be surrounded by a placeholder dummy region+block, no?
If for example we make the unconditional jumps terminators.

Nevermind me, I missed the block added in the diff :)

codecov · 2023-06-21T16:02:58Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (e781c09) 89.34% compared to head (ec35c17) 89.34%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1172      +/-   ##
==========================================
- Coverage   89.34%   89.34%   -0.01%     
==========================================
  Files         181      181              
  Lines       24080    24079       -1     
  Branches     3663     3663              
==========================================
- Hits        21515    21514       -1     
  Misses       1991     1991              
  Partials      574      574

Impacted Files	Coverage Δ
xdsl/dialects/riscv.py	`83.75% <100.00%> (-0.02%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

JosseVanDelm · 2023-06-21T17:30:35Z

I put the return back in the middle because a lot more instructions are about to become terminators so it makes sense for them to be in order again.

I'm sorry I can not follow anymore, what did this PR change?

a lot more instructions are about to become terminators

Because we are going to match the behaviour of CF dialect? With risc-v jmp instruction becoming some type of terminator for example?

superlopuh · 2023-06-22T12:09:28Z

This PR changes the label region to multi-block, and changes the tests to reflect this. Please take a look at my newer PR for more blocks due to branch instructions and j

AntonLydike

Similarly to Josse, I'm having difficulty following what the intentions are here, but if this works for you sure!

AntonLydike · 2023-06-23T09:52:00Z

tests/filecheck/dialects/riscv/riscv_assembly_emission.mlir

+
+    "riscv.j"() {"immediate" = 1 : i32, "rd" = !riscv.reg<zero>} : () -> ()
+    // CHECK-NEXT: j 1
+    "riscv.j"() {"immediate" = #riscv.label<"label">, "rd" = !riscv.reg<zero>} : () -> ()


Shouldn't this then be a block not a label?

that's also in the next PR

AntonLydike · 2023-06-23T09:52:21Z

tests/filecheck/dialects/riscv/riscv_assembly_emission.mlir

+    "riscv.beq"(%2, %1) {"offset" = 1 : i32}: (!riscv.reg<j2>, !riscv.reg<j1>) -> ()
+    // CHECK-NEXT: beq j2, j1, 1
+    "riscv.bne"(%2, %1) {"offset" = 1 : i32}: (!riscv.reg<j2>, !riscv.reg<j1>) -> ()
+    // CHECK-NEXT: bne j2, j1, 1
+    "riscv.blt"(%2, %1) {"offset" = 1 : i32}: (!riscv.reg<j2>, !riscv.reg<j1>) -> ()
+    // CHECK-NEXT: blt j2, j1, 1
+    "riscv.bge"(%2, %1) {"offset" = 1 : i32}: (!riscv.reg<j2>, !riscv.reg<j1>) -> ()
+    // CHECK-NEXT: bge j2, j1, 1
+    "riscv.bltu"(%2, %1) {"offset" = 1 : i32}: (!riscv.reg<j2>, !riscv.reg<j1>) -> ()
+    // CHECK-NEXT: bltu j2, j1, 1
+    "riscv.bgeu"(%2, %1) {"offset" = 1 : i32}: (!riscv.reg<j2>, !riscv.reg<j1>) -> ()
+    // CHECK-NEXT: bgeu j2, j1, 1


Similarly here, shouldn't these be jumping to other blocks?

That's in the next PR, this PR just sets the scene for that change

AntonLydike · 2023-06-23T09:53:22Z

tests/filecheck/dialects/riscv/riscv_ops.mlir

+    "riscv.beq"(%0, %1) {"offset" = 1 : i32}: (!riscv.reg<>, !riscv.reg<>) -> ()
+    // CHECK-NEXT: "riscv.beq"(%{{.*}}, %{{.*}}) {"offset" = 1 : i32} : (!riscv.reg<>, !riscv.reg<>) -> ()
+    "riscv.bne"(%0, %1) {"offset" = 1 : i32}: (!riscv.reg<>, !riscv.reg<>) -> ()
+    // CHECK-NEXT: "riscv.bne"(%{{.*}}, %{{.*}}) {"offset" = 1 : i32} : (!riscv.reg<>, !riscv.reg<>) -> ()
+    "riscv.blt"(%0, %1) {"offset" = 1 : i32}: (!riscv.reg<>, !riscv.reg<>) -> ()
+    // CHECK-NEXT: "riscv.blt"(%{{.*}}, %{{.*}}) {"offset" = 1 : i32} : (!riscv.reg<>, !riscv.reg<>) -> ()
+    "riscv.bge"(%0, %1) {"offset" = 1 : i32}: (!riscv.reg<>, !riscv.reg<>) -> ()
+    // CHECK-NEXT: "riscv.bge"(%{{.*}}, %{{.*}}) {"offset" = 1 : i32} : (!riscv.reg<>, !riscv.reg<>) -> ()
+    "riscv.bltu"(%0, %1) {"offset" = 1 : i32}: (!riscv.reg<>, !riscv.reg<>) -> ()
+    // CHECK-NEXT: "riscv.bltu"(%{{.*}}, %{{.*}}) {"offset" = 1 : i32} : (!riscv.reg<>, !riscv.reg<>) -> ()
+    "riscv.bgeu"(%0, %1) {"offset" = 1 : i32}: (!riscv.reg<>, !riscv.reg<>) -> ()
+    // CHECK-NEXT: "riscv.bgeu"(%{{.*}}, %{{.*}}) {"offset" = 1 : i32} : (!riscv.reg<>, !riscv.reg<>) -> ()


Same thing here

JosseVanDelm

Hi, after the zulip discussion it is now a lot more clear why we need this.
The syntax still looks a bit weird though. Is this me, or is this something that needs fixing?

JosseVanDelm · 2023-06-23T14:57:49Z

tests/filecheck/dialects/riscv/riscv_assembly_emission.mlir

+    // CHECK-NEXT: ebreak
+    "riscv.ret"() : () -> ()
+    // CHECK-NEXT: ret
+    ^1(%b10 : !riscv.reg<>, %b11 : !riscv.reg<>):


Should this be indented --?

JosseVanDelm · 2023-06-23T15:06:37Z

tests/filecheck/dialects/riscv/riscv_assembly_emission.mlir

-  // Unconditional Branch Instructions
-  "riscv.ret"() : () -> () // pseudo-instruction
-  // CHECK-NEXT: ret
+    %0 = "riscv.li"() {"immediate" = 6 : i32} : () -> !riscv.reg<zero>


Shouldn't this start with
^0?

The first block is not printed if it doesn't have arguments, and only block that are printed get labels. It's sort of annoying, I'd like to fix it in the future, but right now that's how it is. I took the labels that xdsl-opt printed and put them on the input so that it wouldn't be confusing.

JosseVanDelm · 2023-06-23T15:13:01Z

tests/filecheck/dialects/riscv/riscv_assembly_emission.mlir

+
+    "riscv.ret"() : () -> ()
+    // CHECK-NEXT: ret
+    ^0(%b00 : !riscv.reg<>, %b01 : !riscv.reg<>):


Should this be indented --?

dedented all

JosseVanDelm · 2023-06-23T15:14:06Z

tests/filecheck/dialects/riscv/riscv_ops.mlir

+
+    "riscv.ret"() : () -> ()
+    // CHECK-NEXT: "riscv.ret"() : () -> ()
+    ^0(%2 : !riscv.reg<>, %3 : !riscv.reg<>):


Should this be indented --?

superlopuh · 2023-06-26T12:43:14Z

We've decided against this direction for the riscv dialect, we'll separate the label in the riscv dialect from its use as a control flow structure, and add this multi-block label at a higher level of abstraction.

superlopuh · 2023-07-02T09:21:18Z

Closing this for now, will probably reopen later

superlopuh · 2023-07-17T15:22:42Z

@compor @AntonLydike can you please re-review?

compor · 2023-07-18T07:20:44Z

Considering that using both our paths to lower required this I'm happy to accept it.
There are a few things to explore, but AFAIC this is now desirable.

superlopuh added the dialects Changes on the dialects label Jun 21, 2023

superlopuh requested review from nazavode, AntonLydike and JosseVanDelm June 21, 2023 13:58

superlopuh self-assigned this Jun 21, 2023

superlopuh requested review from compor, math-fehr, webmiche and georgebisbas as code owners June 21, 2023 13:58

superlopuh force-pushed the sasha/riscv/label-multi-block branch from 8a72b03 to 9a62634 Compare June 21, 2023 15:58

compor approved these changes Jun 21, 2023

View reviewed changes

AntonLydike reviewed Jun 23, 2023

View reviewed changes

JosseVanDelm reviewed Jun 23, 2023

View reviewed changes

superlopuh requested review from JosseVanDelm and AntonLydike June 23, 2023 20:36

superlopuh marked this pull request as draft June 26, 2023 12:43

superlopuh added 3 commits June 27, 2023 21:36

dialects: (riscv) label region is multi block

623b67f

add return in the label region

5a19eb5

dedent

4c7335c

superlopuh force-pushed the sasha/riscv/label-multi-block branch from 03a8d1c to 4c7335c Compare June 27, 2023 20:36

superlopuh marked this pull request as ready for review June 27, 2023 20:36

superlopuh closed this Jul 2, 2023

superlopuh reopened this Jul 17, 2023

Merge branch 'main' into sasha/riscv/label-multi-block

ec35c17

superlopuh mentioned this pull request Jul 17, 2023

dialects: (riscv) make LabelOp region multi-block #1282

Closed

compor self-requested a review July 17, 2023 15:26

compor approved these changes Jul 18, 2023

View reviewed changes

superlopuh merged commit 7b597fc into main Jul 19, 2023
10 checks passed

superlopuh deleted the sasha/riscv/label-multi-block branch July 19, 2023 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (riscv) label region is multi block #1172

dialects: (riscv) label region is multi block #1172

superlopuh commented Jun 21, 2023

compor commented Jun 21, 2023

superlopuh commented Jun 21, 2023

compor commented Jun 21, 2023 •

edited

codecov bot commented Jun 21, 2023 •

edited

JosseVanDelm commented Jun 21, 2023

superlopuh commented Jun 22, 2023

AntonLydike left a comment

AntonLydike Jun 23, 2023

superlopuh Jun 23, 2023

AntonLydike Jun 23, 2023

superlopuh Jun 23, 2023

AntonLydike Jun 23, 2023

JosseVanDelm left a comment

JosseVanDelm Jun 23, 2023

JosseVanDelm Jun 23, 2023

superlopuh Jun 23, 2023

JosseVanDelm Jun 23, 2023

superlopuh Jun 23, 2023

JosseVanDelm Jun 23, 2023

superlopuh commented Jun 26, 2023

superlopuh commented Jul 2, 2023

superlopuh commented Jul 17, 2023

compor commented Jul 18, 2023

dialects: (riscv) label region is multi block #1172

dialects: (riscv) label region is multi block #1172

Conversation

superlopuh commented Jun 21, 2023

compor commented Jun 21, 2023

superlopuh commented Jun 21, 2023

compor commented Jun 21, 2023 • edited

codecov bot commented Jun 21, 2023 • edited

Codecov Report

JosseVanDelm commented Jun 21, 2023

superlopuh commented Jun 22, 2023

AntonLydike 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

JosseVanDelm 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

superlopuh commented Jun 26, 2023

superlopuh commented Jul 2, 2023

superlopuh commented Jul 17, 2023

compor commented Jul 18, 2023

compor commented Jun 21, 2023 •

edited

codecov bot commented Jun 21, 2023 •

edited