Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIR][CodeGen] VLA support next step #453

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Conversation

gitoleg
Copy link
Collaborator

@gitoleg gitoleg commented Feb 6, 2024

Here is the next step in VLA support.
Basically, these changes handle different expressions, like int (*a[5])[n] or sizeof(a[n]).

I took tests from the original codegen - they don't check anything, just verify we don't fail.

There is still an issue with a proper cleanup - there are cases when stack_save doesn't dominate a corresponded stack_restore. For example in the next example:

void test(unsigned x) {
  while (1) {
    char a[x];
    if (x > 5) 
      break;
    ++x;
  }
}

Look like break here doesn't lead to stack_restore. But I would say this is less related to VLA, though probably I need to fix this as well.

Btw, looks like somehow I didn't attach tests last time, my bad!

Copy link

github-actions bot commented Feb 6, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@bcardosolopes
Copy link
Member

Look like break here doesn't lead to stack_restore. But I would say this is less related to VLA, though probably I need to fix this as well.

Verifiers doing some nice work here!

This is right, this is not VLA specific. One thing I'm doing in this front is to make sure all cleanup things at least stay in the scope "cleanup" block so that whenever I fix this issue, it can apply to all relevant cases. That said, I don't think during past reviews for cir.stack_restore I asked you to do place it in that block. Can you change this? (Doesn't need to be part of this PR).

@bcardosolopes
Copy link
Member

For example in the next example

Can you also insert the example in the testcase and leave it commented out with a FIXME?

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bcardosolopes bcardosolopes merged commit ed08d6f into llvm:main Feb 8, 2024
6 checks passed
@gitoleg
Copy link
Collaborator Author

gitoleg commented Feb 9, 2024

@bcardosolopes
hey, sorry for the late reply, my bad!

That said, I don't think during past reviews for cir.stack_restore I asked you to do place it in that block. Can you change this?

I'm not sure I understand you, my bad! Now the clean up for VLA is inserted here, and as far I understand this is the only place where we need to insert it.
So, again, what do you think I can do?

@bcardosolopes
Copy link
Member

Hi Oleg.

I'm not sure I understand you, my bad! Now the clean up for VLA is inserted here, and as far I understand this is the only place where we need to insert it.

No problem, seems like it might already be in the right place.

So, again, what do you think I can do?

BTW, I ran into an issue with vla.c yesterday related with cleanups when adding more skeletons to complete function epilogue, see the FIXME's in eaf965e. This is hinting to the possibility that such cleanups could be slightly wrong, or there's something missing in this PR.

Anyways, if you have hints about this, just let me know - but I'm gonna take a look at the issue, no sweat for you. Thanks for offering help!

lanza pushed a commit that referenced this pull request Mar 23, 2024
Here is the next step in VLA support.
Basically, these changes handle different expressions, like `int
(*a[5])[n]` or `sizeof(a[n])`.

I took tests from the original `codegen` - they don't check anything,
just verify we don't fail.

There is still an issue with a proper cleanup - there are cases when
`stack_save` doesn't dominate a corresponded `stack_restore`. For
example in the next example:

```
void test(unsigned x) {
  while (1) {
    char a[x];
    if (x > 5) 
      break;
    ++x;
  }
}
```
Look like `break` here doesn't lead to `stack_restore`. But I would say
this is less related to VLA, though probably I need to fix this as well.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Mar 24, 2024
Here is the next step in VLA support.
Basically, these changes handle different expressions, like `int
(*a[5])[n]` or `sizeof(a[n])`.

I took tests from the original `codegen` - they don't check anything,
just verify we don't fail.

There is still an issue with a proper cleanup - there are cases when
`stack_save` doesn't dominate a corresponded `stack_restore`. For
example in the next example:

```
void test(unsigned x) {
  while (1) {
    char a[x];
    if (x > 5) 
      break;
    ++x;
  }
}
```
Look like `break` here doesn't lead to `stack_restore`. But I would say
this is less related to VLA, though probably I need to fix this as well.
lanza pushed a commit that referenced this pull request Apr 29, 2024
Here is the next step in VLA support.
Basically, these changes handle different expressions, like `int
(*a[5])[n]` or `sizeof(a[n])`.

I took tests from the original `codegen` - they don't check anything,
just verify we don't fail.

There is still an issue with a proper cleanup - there are cases when
`stack_save` doesn't dominate a corresponded `stack_restore`. For
example in the next example:

```
void test(unsigned x) {
  while (1) {
    char a[x];
    if (x > 5)
      break;
    ++x;
  }
}
```
Look like `break` here doesn't lead to `stack_restore`. But I would say
this is less related to VLA, though probably I need to fix this as well.
lanza pushed a commit that referenced this pull request Apr 29, 2024
Here is the next step in VLA support.
Basically, these changes handle different expressions, like `int
(*a[5])[n]` or `sizeof(a[n])`.

I took tests from the original `codegen` - they don't check anything,
just verify we don't fail.

There is still an issue with a proper cleanup - there are cases when
`stack_save` doesn't dominate a corresponded `stack_restore`. For
example in the next example:

```
void test(unsigned x) {
  while (1) {
    char a[x];
    if (x > 5)
      break;
    ++x;
  }
}
```
Look like `break` here doesn't lead to `stack_restore`. But I would say
this is less related to VLA, though probably I need to fix this as well.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Apr 29, 2024
Here is the next step in VLA support.
Basically, these changes handle different expressions, like `int
(*a[5])[n]` or `sizeof(a[n])`.

I took tests from the original `codegen` - they don't check anything,
just verify we don't fail.

There is still an issue with a proper cleanup - there are cases when
`stack_save` doesn't dominate a corresponded `stack_restore`. For
example in the next example:

```
void test(unsigned x) {
  while (1) {
    char a[x];
    if (x > 5) 
      break;
    ++x;
  }
}
```
Look like `break` here doesn't lead to `stack_restore`. But I would say
this is less related to VLA, though probably I need to fix this as well.
lanza pushed a commit that referenced this pull request Apr 29, 2024
Here is the next step in VLA support.
Basically, these changes handle different expressions, like `int
(*a[5])[n]` or `sizeof(a[n])`.

I took tests from the original `codegen` - they don't check anything,
just verify we don't fail.

There is still an issue with a proper cleanup - there are cases when
`stack_save` doesn't dominate a corresponded `stack_restore`. For
example in the next example:

```
void test(unsigned x) {
  while (1) {
    char a[x];
    if (x > 5)
      break;
    ++x;
  }
}
```
Look like `break` here doesn't lead to `stack_restore`. But I would say
this is less related to VLA, though probably I need to fix this as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants