[WIP] dialects (arm): add cf instructions #3744

emmau678 · 2025-01-13T10:12:04Z

Add control flow instructions for ARM (same as those already implemented for riscv).
Please note: Not ready for review, this is a WIP branch. I will break this up into smaller PRs and eventually delete this one

emmau678 · 2025-01-13T10:15:49Z

xdsl/dialects/arm_cf.py

+
+    then_block = successor_def()
+
+    assembly_format = (


I'm aware that this is not correct, but I'm not sure how exactly to fix it. In arm, beq must follow a cmp in the line beforehand. (Related question: Shall I still treat is as one instruction and just include the cmp in the printing, or implement cmp separately?)

I think I'd add the cmp separately and have a verification check here in beq the enforces this constraint (i.e., the cmp must be the instruction prior to this), similarly to some of the verify_ methods in the RISC-V dialect.
We don't have many there and the constraints are a bit more involved, but you should get an idea.

Let me know if you have more questions on this.

I'm not 100% sure, if the constraint is so hard-coded, then maybe we could get away with a single op. Probably worth a discussion.

Well, it depends on the CPSR AFAIU, and I'm not sure how to model that fully, e.g., when these are cleared, as the specification phrases this as "set by the last flag-setting instruction... etc.". For example, the cmp instruction sets the Z flag.

compor

Overall looks good, I left some comments. I expect the Label op and attribute PRs to go in first, which would simplify this one.

Maybe also rename the PR title to be the specific control flow instruction that you intend to add here?

compor · 2025-01-13T12:04:52Z

xdsl/dialects/arm_cf.py

+
+    then_block = successor_def()
+
+    assembly_format = (


I think I'd add the cmp separately and have a verification check here in beq the enforces this constraint (i.e., the cmp must be the instruction prior to this), similarly to some of the verify_ methods in the RISC-V dialect.
We don't have many there and the constraints are a bit more involved, but you should get an idea.

Let me know if you have more questions on this.

emmau678 · 2025-01-13T15:29:07Z

Overall looks good, I left some comments. I expect the Label op and attribute PRs to go in first, which would simplify this one.

Maybe also rename the PR title to be the specific control flow instruction that you intend to add here?

thanks for the review @compor. This was my initial attempt at adding support for CF but I felt it was getting a bit too long for a single PR, so I pushed it as a WIP branch so I could get some feedback. I'm planning to break it up into a couple of smaller PRs, so I'll probably end up cutting this down/deleting it afterwards

compor · 2025-01-13T16:09:55Z

Overall looks good, I left some comments. I expect the Label op and attribute PRs to go in first, which would simplify this one.
Maybe also rename the PR title to be the specific control flow instruction that you intend to add here?

thanks for the review @compor. This was my initial attempt at adding support for CF but I felt it was getting a bit too long for a single PR, so I pushed it as a WIP branch so I could get some feedback. I'm planning to break it up into a couple of smaller PRs, so I'll probably end up cutting this down/deleting it afterwards

Excellent @emmau678, no objection. Happy to hear and discuss your thoughts on the above discussion on the cmp/CPSR issue.

alexarice

Left some comment but not 100% they are very useful

alexarice · 2025-01-15T12:27:01Z

tests/filecheck/dialects/arm_cf/test_ops.mlir

+// RUN: XDSL_GENERIC_ROUNDTRIP
+// RUN: xdsl-opt -t arm-asm %s | filecheck %s --check-prefix=CHECK-ASM
+
+module {


It surprises me that this verifies, as ModuleOp is tagged as being single_block

alexarice · 2025-01-15T12:28:51Z

xdsl/dialects/arm/__init__.py

@@ -8,7 +8,7 @@
 from xdsl.dialects.builtin import ModuleOp
 from xdsl.ir import Dialect

-from .ops import ARMOperation, DSMovOp, DSSMulOp, GetRegisterOp
+from .ops import ARMOperation, DSMovOp, DSSMulOp, GetRegisterOp, LabelOp


Feels like this could be its own PR

this is actually its own PR now (#3749), and I think all of your other comments are resolved in that one. apologies for the confusion, I will clearly mark WIP PRs from now on

alexarice · 2025-01-15T12:30:04Z

xdsl/dialects/arm/assembly.py

-AssemblyInstructionArg: TypeAlias = SSAValue
+
+@irdl_attr_definition
+class LabelAttr(Data[str]):


Why not just use a StringAttr here?

alexarice · 2025-01-15T12:33:09Z

xdsl/dialects/arm/ops.py

+    label = attr_def(LabelAttr)
+    comment = opt_attr_def(StringAttr)
+
+    assembly_format = "attr-dict"


Should this not have label in it somewhere?

alexarice · 2025-01-15T12:33:20Z

xdsl/dialects/arm/ops.py

+    """
+
+    name = "arm.label"
+    label = attr_def(LabelAttr)


any reason this isn't a property?

emmau678 · 2025-01-15T14:25:25Z

Overall looks good, I left some comments. I expect the Label op and attribute PRs to go in first, which would simplify this one.
Maybe also rename the PR title to be the specific control flow instruction that you intend to add here?

thanks for the review @compor. This was my initial attempt at adding support for CF but I felt it was getting a bit too long for a single PR, so I pushed it as a WIP branch so I could get some feedback. I'm planning to break it up into a couple of smaller PRs, so I'll probably end up cutting this down/deleting it afterwards

@alexarice, thanks for the review also! this was very much a WIP when I pushed it and I have since done a smaller PR as a first step LabelOp. I'm planning to build on this incrementally in further PRs. I have updated the PR description and will do this next time if I push a WIP PR

superlopuh · 2025-01-15T14:55:07Z

We tend to use [WIP] in the PR title to mark PRs as such

dialects (arm): (broken commit) add initial attempt at beq instruction

54a9c99

emmau678 added the dialects Changes on the dialects label Jan 13, 2025

emmau678 requested a review from superlopuh January 13, 2025 10:12

emmau678 self-assigned this Jan 13, 2025

emmau678 commented Jan 13, 2025

View reviewed changes

compor reviewed Jan 13, 2025

View reviewed changes

alexarice reviewed Jan 15, 2025

View reviewed changes

emmau678 changed the title ~~dialects (arm): add cf instructions~~ [WIP] dialects (arm): add cf instructions Jan 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] dialects (arm): add cf instructions #3744

[WIP] dialects (arm): add cf instructions #3744

emmau678 commented Jan 13, 2025 •

edited

Loading

emmau678 Jan 13, 2025

compor Jan 13, 2025

superlopuh Jan 13, 2025

compor Jan 13, 2025

compor left a comment

compor Jan 13, 2025

emmau678 commented Jan 13, 2025

compor commented Jan 13, 2025

alexarice left a comment

alexarice Jan 15, 2025

alexarice Jan 15, 2025

emmau678 Jan 15, 2025

alexarice Jan 15, 2025

alexarice Jan 15, 2025

alexarice Jan 15, 2025

emmau678 commented Jan 15, 2025 •

edited

Loading

superlopuh commented Jan 15, 2025

[WIP] dialects (arm): add cf instructions #3744

Are you sure you want to change the base?

[WIP] dialects (arm): add cf instructions #3744

Conversation

emmau678 commented Jan 13, 2025 • 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

compor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmau678 commented Jan 13, 2025

compor commented Jan 13, 2025

alexarice 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

emmau678 commented Jan 15, 2025 • edited Loading

superlopuh commented Jan 15, 2025

emmau678 commented Jan 13, 2025 •

edited

Loading

emmau678 commented Jan 15, 2025 •

edited

Loading