[CIR][CodeGen] Adds support for inline assembler #308

gitoleg · 2023-11-09T14:54:08Z

This PR adds support for the inline assembly in CIR.

The PR is explicitly marked as a draft, since some discussion needed.

As usually, I started with the original codegen (clang/lib/Codegen), and here is a first thing we need to agree on. The original code is kind of hard to read and the first desire is to refactor the code in this PR so that it can be readable or maintainable or both. Or at least do something - actually I already did some preliminary things to clean up a little.

Another approach - we may leave the code closer to the original one in order to make it simpler to reflect all the future changes in the original codegen in CIR. So it's up to you to decide which approach to apply.

Now, the PR itself.

First of all, there are several unimplemented features and limitations:

Goto. Just because a destination may be in another region and we don't want to emit branches like that.
We don't add attributes that describe memory effect, though it can be done, but will require some changes in the lowering from MLIR to LLVM IR.
No support for intrinsic like llvm.assume or bundles.
We don't support vector types so far, since CIR doesn't support them either.
There are some virtual functions that should be defined for each particular architecture - and we don't do it so far and use the default behavior.
No support for module level asm

Tests

The main question is about tests. As usually we want to have such tests, that cover all use cases, every branch of the code.
But the point is - we don't need to repeat all the assembler tests from the original codegen. E.g. we don't need to check how the parse functions work. We may assume that such things are checked there.

The same is true for the different architectures support. Probably, we will need to add something arch-specific in some point of time. But it's ok for now to check everything just for x86. By "everything" I mean the code, that is CIR specific, give or take, e.g. we emit the value/operation where we need to.

Thus, for now I added just several tests and I kind of feel shame for this small number of checks I do. I'm sure we want to have more tests. And I will definitely add more. But in the same time I'm sure we don't need too much. So, I would be happy to hear your point of view.

So far, some trivial examples work and you are welcome to check (or break something :) )

UPD:
Added tests from the the clang/test/CodeGen/asm.c with no checks included (so far). Just to make sure we don't crush in these scenario. Only three of them skipped: module level asm, vector types and goto.

bcardosolopes · 2023-11-17T22:10:28Z

Thanks for contributing support for this in ClangIR, much welcome feature.

Another approach - we may leave the code closer to the original one in order to make it simpler to reflect all the future changes in the original codegen in CIR. So it's up to you to decide which approach to apply.

This one. I rather do it closer to original (to the most extend as possible) - easy for new people getting involved in the project to catch up and help.

First of all, there are several unimplemented features and limitations:

Goto. Just because a destination may be in another region and we don't want to emit branches like that.

We don't add attributes that describe memory effect, though it can be done, but will require some changes in the lowering from MLIR to LLVM IR.

No support for intrinsic like llvm.assume or bundles.

We don't support vector types so far, since CIR doesn't support them either.

There are some virtual functions that should be defined for each particular architecture - and we don't do it so far and use the default behavior.

No support for module level asm

This is fine, let's go incremental here.

Tests

The main question is about tests. As usually we want to have such tests, that cover all use cases, every branch of the code. But the point is - we don't need to repeat all the assembler tests from the original codegen. E.g. we don't need to check how the parse functions work. We may assume that such things are checked there.

You could also copy the tests from the original codegen and make sure it passes.

The same is true for the different architectures support. Probably, we will need to add something arch-specific in some point of time. But it's ok for now to check everything just for x86. By "everything" I mean the code, that is CIR specific, give or take, e.g. we emit the value/operation where we need to.

As long as you add an assert for other arches, x86 is fine.

Thus, for now I added just several tests and I kind of feel shame for this small number of checks I do. I'm sure we want to have more tests. And I will definitely add more. But in the same time I'm sure we don't need too much. So, I would be happy to hear your point of view.

My take here is to add some simple tests first and later start copy-n-paste the original ones, and make sure they pass.

So far, some trivial examples work and you are welcome to check (or break something :) )

UPD: Added tests from the the clang/test/CodeGen/asm.c with no checks included (so far). Just to make sure we don't crush in these scenario. Only three of them skipped: module level asm, vector types and goto.

Gotcha!

Big patches are hard to review, I'm currently the only one doing PR reviews, so if we go even more incrementally it helps me build context and accept your patches faster.

Can you break this down further? For example, the first PR could just add empty asm string, a very simple initial operation without all operands or attributes. Each following PR can then add constraints, etc. You can still keep this PR open with the complete work you have already, and slowly build the feature upstream one little piece at a time.

gitoleg · 2023-11-20T10:49:39Z

Can you break this down further?

Well, hard task! Let's try to do it.

I will break the PR #308 into pieces and submit them one-by-one. The first PR introduce CIR operation and the `buildAsmStmt` function. The latter is the main place for the future changesm and the former was taken directly from MLIR LLVM IR dialect. As a result, there is nothing really interesting happen here, but now we can at least emit cir for an empty inline assembler. And as a first step

Guards cir.scope lowering against empty scopes avoiding segfaults. Also removes empty cir.scope operations on MergeCleanups pass. ghstack-source-id: c453955 Pull Request resolved: llvm#144

Essentially converts a `cir.loop` op of the `while` kind to a CFG. The implementation, however, was only tested with structured loops, so if breaks, continues, or returns are found in the body, it is likely to break. ghstack-source-id: 32d2624 Pull Request resolved: llvm#145

Conditionally set the loop entry point depending on whether the loop is of the `do-while` kind or not. ghstack-source-id: 711f4be Pull Request resolved: llvm#146

ghstack-source-id: 8e98e12 Pull Request resolved: llvm#147

globals.cir should be fixed, goto.cir is dead given that we aren't supporting ThroughMLIR

In C, whenever we logically compare two values, we may store the result in any variable type. In this case, the boolean result should be cast to whatever type the variable is. This patch adds support for this.

Casts from one integer type to another, should not be lowered to LLVM if the integers are of the same size. These may represent sign drops or inclusion in CIR but do not make sense in LLVM. This patch fixes that. ghstack-source-id: 0b02850 Pull Request resolved: llvm#154

We should not have tests that depend on target-specific executables. This patch removes the 'hello.c' executable test and converts it to both a codegen and a lowering test. ghstack-source-id: caa8f3a Pull Request resolved: llvm#155

This refactor merges the lowering logic of all the different kinds of loops into a single function. It also removes unnecessary LIT tests that validate LLVM dialect to LLVM IR lowering, as this functionality is not within CIR's scope. Fixes llvm#153 ghstack-source-id: ebaab85 Pull Request resolved: llvm#156

ghstack-source-id: 16b236e Pull Request resolved: llvm#157

…lineAttr (llvm#134) Setting inline attributes based on user input and command line options. This is optional as functions do not need such an attribute will not get the attribute.

… assist clangir dev

cir.binop has the constraints on operands and results types than cannot support shifts with amount types different from the value getting shift. Adding cir.shift is a closer map.

…fferences

ghstack-source-id: 9d59b38 Pull Request resolved: llvm#162

LLVM's dialect does not have (at the time of writing) a way to represent zero-initializers. And, despite being possible to represent null pointers, it requires it to use a region-based initialization. To avoid this, LLVM operations that use either of these attributes are marked with a cir.zero attribute that will be identified by the LowerAttrToLLVMIR interface and patch the LLVM IR operation to be initialized with a zero-initializer or null. ghstack-source-id: 1872476 Pull Request resolved: llvm#163

Incremental work towards tracking exploded structs, no testcase just yet.

This change introduces `cir-translate` as a replacement of `mlir-translate` to convert CIR directly to LLVM IR. The main benefit of this is to utilize the CIR attribute interface to handle CIR-specific attributes such as `cir.extra`. Other advantages at this time, besides the cir attribute support, could be that we can go directly from CIR to LLVMIR without exposing the intermediate MLIR LLVM dialect form. Previously `cir-tool` emit the LLVM dialect form and `milr-translate` took it from there. Now `cir-translate` can directly take CIR and yield LLVMIR. I'm also renaming `cir-tool` to `cir-opt` which eventually would be just a CIR-to-CIR transformer, but for now I'm keeping the functionality of CIR to LLVM dialect. So, `cir-opt` will do all CIR-to-CIR transforms, just like LLVM `opt` or `mlir-opt`, and `cir-translate` will handle CIR to LLVMIR translation, and LLVMIR-to-LLVMIR transforms, like the LLVM `llc` or the `mlir-translate`

…vm#356) This PR adds a support for the multi-block case statements. Previously, the code example below caused crash in cir verification Lowering to the `llvm` dialect is pretty straightforward: the same logic as before is applied to all the region's blocks with no successors, i.e. we no longer think a case/default region contains only one block. The `CodeGen` part is a little bit tricky. Previously, any sub-statement of `case` or`default`, that was not any of them (i.e. neither `case` nor `default`) was processed with an insertion guard, meaning that the next sub-statement in the same clause was inserted again in the same block as the first one. It would be fine, once sub-statement didn't generate any blocks as well. For instance, ``` void foo(int a) { switch (a) { case 3: return; break; } } ``` The `return` statement actually emit a new block after, where the unreachable code with `break` should be inserted in. That's why we also need to update `lastCaseBlock` while generating `cir.switch` This is quite frequent bug in `llvm-test-suite`

This contains just the skeleton, but the idea is that this pass is going to contain transformations done on top CIR generated by the C/C++ idiom recognizer.

…tream

I will break the PR #308 into pieces and submit them one-by-one. The first PR introduce CIR operation and the `buildAsmStmt` function. The latter is the main place for the future changesm and the former was taken directly from MLIR LLVM IR dialect. As a result, there is nothing really interesting happen here, but now we can at least emit cir for an empty inline assembler. And as a first step

I will break the PR llvm#308 into pieces and submit them one-by-one. The first PR introduce CIR operation and the `buildAsmStmt` function. The latter is the main place for the future changesm and the former was taken directly from MLIR LLVM IR dialect. As a result, there is nothing really interesting happen here, but now we can at least emit cir for an empty inline assembler. And as a first step

bcardosolopes · 2024-04-17T20:18:10Z

@gitoleg can we close this?

gitoleg · 2024-04-18T07:37:27Z

The PR can be closed now, everything is implemented in the next PRs:
#326 #351 #465 #469 #472 #491 #512

I will break the PR #308 into pieces and submit them one-by-one. The first PR introduce CIR operation and the `buildAsmStmt` function. The latter is the main place for the future changesm and the former was taken directly from MLIR LLVM IR dialect. As a result, there is nothing really interesting happen here, but now we can at least emit cir for an empty inline assembler. And as a first step

I will break the PR llvm#308 into pieces and submit them one-by-one. The first PR introduce CIR operation and the `buildAsmStmt` function. The latter is the main place for the future changesm and the former was taken directly from MLIR LLVM IR dialect. As a result, there is nothing really interesting happen here, but now we can at least emit cir for an empty inline assembler. And as a first step

I will break the PR #308 into pieces and submit them one-by-one. The first PR introduce CIR operation and the `buildAsmStmt` function. The latter is the main place for the future changesm and the former was taken directly from MLIR LLVM IR dialect. As a result, there is nothing really interesting happen here, but now we can at least emit cir for an empty inline assembler. And as a first step

I will break the PR llvm#308 into pieces and submit them one-by-one. The first PR introduce CIR operation and the `buildAsmStmt` function. The latter is the main place for the future changesm and the former was taken directly from MLIR LLVM IR dialect. As a result, there is nothing really interesting happen here, but now we can at least emit cir for an empty inline assembler. And as a first step

I will break the PR #308 into pieces and submit them one-by-one. The first PR introduce CIR operation and the `buildAsmStmt` function. The latter is the main place for the future changesm and the former was taken directly from MLIR LLVM IR dialect. As a result, there is nothing really interesting happen here, but now we can at least emit cir for an empty inline assembler. And as a first step

gitoleg mentioned this pull request Nov 20, 2023

[CIR][CodeGen] inline assembler: build CIR operation #326

Merged

lanza force-pushed the main branch from 84c3bc5 to 05ffb2a Compare December 20, 2023 06:33

bcardosolopes and others added 25 commits December 19, 2023 22:37

[CIR][CIRGen] VisitUnaryExprOrTypeTraitExpr: implement it for scalars

3a92ffa

[CIR][Lowering] Handle empty scope operations

a1d7a99

Guards cir.scope lowering against empty scopes avoiding segfaults. Also removes empty cir.scope operations on MergeCleanups pass. ghstack-source-id: c453955 Pull Request resolved: llvm#144

[CIR][Lowering] Lower structured do-while loops

8ddc743

Conditionally set the loop entry point depending on whether the loop is of the `do-while` kind or not. ghstack-source-id: 711f4be Pull Request resolved: llvm#146

[CIR] Add reconcile unrealized casts pass

d9def5e

ghstack-source-id: 8e98e12 Pull Request resolved: llvm#147

[CIR][Rebase] Fix a bunch of llvm::Optional -> std::optional issues

29fa484

[CIR][Rebase] XFAIL a few tests

04b019b

globals.cir should be fixed, goto.cir is dead given that we aren't supporting ThroughMLIR

[CIR][Lowering] Add dep for CIREnumsGen

4bfcf23

[CIR] Add MLIRBuiltinLocationAttributesIncGen to deps for FrontendTool

d0db2f4

[CIR][CodeGen] Support conditional result implicit cast

ef940d3

In C, whenever we logically compare two values, we may store the result in any variable type. In this case, the boolean result should be cast to whatever type the variable is. This patch adds support for this.

[CIR] Remove the Executables tests folder

a6e8c1d

We should not have tests that depend on target-specific executables. This patch removes the 'hello.c' executable test and converts it to both a codegen and a lowering test. ghstack-source-id: caa8f3a Pull Request resolved: llvm#155

[CIR][Lowering][NFC] Inline rewriteLoop method

e627757

ghstack-source-id: 16b236e Pull Request resolved: llvm#157

[CIR][CIRGen] Add threshold for recursive buildDeferred

8fa4c88

[CIR][CIRGen][Lowering] Introduce, use and lower ExtraFuncAttr and In…

56fbe65

…lineAttr (llvm#134) Setting inline attributes based on user input and command line options. This is optional as functions do not need such an attribute will not get the attribute.

[CIR][CIRGen] Fix previous added flag and add testcase

f46fd71

[CIR][CIRGen] -fclangir-skip-system-headers: internal codegen flag to…

d89ae77

… assist clangir dev

[CIR][CIRGen] Add cir.shift and remove kinds from cir.binop

8d88ce7

cir.binop has the constraints on operands and results types than cannot support shifts with amount types different from the value getting shift. Adding cir.shift is a closer map.

[CIR][CIRGen] Refactor buildShr and buildShl and highlight codegen di…

45b12ef

…fferences

[CIR][CIRGen] Generate zero initializer for pointers and structs

3c710bf

ghstack-source-id: 9d59b38 Pull Request resolved: llvm#162

[CIR][LifetimeCheck] Detect aggregate categories on top of allocas

2d8239b

Incremental work towards tracking exploded structs, no testcase just yet.

[CIR][LifetimeCheck][NFC] Factor out type category classification

1915e7c

gitoleg and others added 5 commits December 19, 2023 22:37

[CIR][NFC] Fix some copy n paste and update comments

1e433e0

[CIR][Passes] Introduce cir-lib-opt pass

6baac6c

This contains just the skeleton, but the idea is that this pass is going to contain transformations done on top CIR generated by the C/C++ idiom recognizer.

[CIR] replace startswith with starts_with for deprecation

5cdf34c

[CIR][Lowering] Add an argument to a overridden function to match ups…

34fceae

…tream

lanza force-pushed the main branch from 05ffb2a to 34fceae Compare December 20, 2023 07:10

[CIR][CodeGen][Lowering] adds inline assembly support

bc1f28f

gitoleg force-pushed the asm branch from c3a698b to bc1f28f Compare December 21, 2023 09:10

lanza force-pushed the main branch 2 times, most recently from 4e069c6 to 79d4dc7 Compare January 29, 2024 23:34

lanza force-pushed the main branch 2 times, most recently from ed3955a to 8b7417c Compare March 23, 2024 05:07

lanza force-pushed the main branch from 8b7417c to 330c983 Compare March 30, 2024 04:29

lanza force-pushed the main branch from 93715bd to a8b0ecd Compare April 17, 2024 05:15

gitoleg closed this Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[CIR][CodeGen] Adds support for inline assembler #308

[CIR][CodeGen] Adds support for inline assembler #308

Uh oh!

gitoleg commented Nov 9, 2023 •

edited

Loading

Uh oh!

bcardosolopes commented Nov 17, 2023

Tests

Uh oh!

gitoleg commented Nov 20, 2023

Uh oh!

bcardosolopes commented Apr 17, 2024

Uh oh!

gitoleg commented Apr 18, 2024

Uh oh!

Uh oh!

[CIR][CodeGen] Adds support for inline assembler #308

[CIR][CodeGen] Adds support for inline assembler #308

Uh oh!

Conversation

gitoleg commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Uh oh!

bcardosolopes commented Nov 17, 2023

Tests

Uh oh!

gitoleg commented Nov 20, 2023

Uh oh!

bcardosolopes commented Apr 17, 2024

Uh oh!

gitoleg commented Apr 18, 2024

Uh oh!

Uh oh!

gitoleg commented Nov 9, 2023 •

edited

Loading