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.

Sign up for GitHub

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

Jump to bottom

[RISCV][Clang] Added code to generate clang error if immediate operan… #93

Merged
merged 6 commits into from
Dec 27, 2023
Merged

[RISCV][Clang] Added code to generate clang error if immediate operan… #93

merged 6 commits into from
Dec 27, 2023

Conversation

adeel10x
Copy link
Contributor

…d of the following builtins is out of range:

  • __builtin_riscv_cv_simd_extract_h
  • __builtin_riscv_cv_simd_extract_b
  • __builtin_riscv_cv_simd_extractu_h
  • __builtin_riscv_cv_simd_extractu_b
  • __builtin_riscv_cv_simd_insert_h
  • __builtin_riscv_cv_simd_insert_b

@adeel10x
[RISCV][Clang] Added code to generate clang error if immediate operan…
912fb12 
…d of the following builtins is out of range:

-  __builtin_riscv_cv_simd_extract_h
-  __builtin_riscv_cv_simd_extract_b
-  __builtin_riscv_cv_simd_extractu_h
-  __builtin_riscv_cv_simd_extractu_b
-  __builtin_riscv_cv_simd_insert_h
-  __builtin_riscv_cv_simd_insert_b
@adeel10x
clang formatted
1c372bc
@PaoloS02 PaoloS02 mentioned this pull request Dec 12, 2023
Merged
PaoloS02
PaoloS02 reviewed Dec 12, 2023
View reviewed changes
clang/lib/Sema/SemaChecking.cpp Outdated
case RISCVCOREV::BI__builtin_riscv_cv_simd_extract_b:
case RISCVCOREV::BI__builtin_riscv_cv_simd_extractu_h:
case RISCVCOREV::BI__builtin_riscv_cv_simd_extractu_b:
return SemaBuiltinConstantArgRange(TheCall, 1, 0, 63);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should use 63 as range limit. These instructions are supposed to be used with values [0,1,2,3] for *_b and [0,1] *_h to select the bytes or half words. I'd make that clear straight away in the front-end, otherwise we may need to truncate or crash or silently mis-select the byte/half word later. So I'd say:
for extract_h and extractu_h:

  return SemaBuiltinConstantArgRange(TheCall, 1, 0, 1);

while for extract_b and extractu_b:

  return SemaBuiltinConstantArgRange(TheCall, 1, 0, 3);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I'll make the changes accordingly.

PaoloS02
PaoloS02 reviewed Dec 12, 2023
View reviewed changes
clang/lib/Sema/SemaChecking.cpp Outdated
return SemaBuiltinConstantArgRange(TheCall, 1, 0, 63);
case RISCVCOREV::BI__builtin_riscv_cv_simd_insert_h:
case RISCVCOREV::BI__builtin_riscv_cv_simd_insert_b:
return SemaBuiltinConstantArgRange(TheCall, 2, 0, 63);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same fix as above for the range

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have fixed the ranges accordingly. I just noticed that I have run clang-format on the files. Is it fine or should I reverse it?

@PaoloS02
Copy link
Contributor

I've added a couple of comments but otherwise it looks good. Could you also check whether there are Sema tests for this kind of validations? We could do with a test for these

@PaoloS02
Copy link
Contributor

I've added a couple of comments but otherwise it looks good. Could you also check whether there are Sema tests for this kind of validations? We could do with a test for these

I see there are not many Sema tests so probably the best way to test the errors is to add cases to CodeGen/RISCV/corev-intrinsics/simd.c where we check that the error message is generated.

@adeel10x
Copy link
Contributor Author

I've added a couple of comments but otherwise it looks good. Could you also check whether there are Sema tests for this kind of validations? We could do with a test for these

I see there are not many Sema tests so probably the best way to test the errors is to add cases to CodeGen/RISCV/corev-intrinsics/simd.c where we check that the error message is generated.

ok sure.

@adeel10x
[RISCV][Clang]Fixed immediate operand range for the following intrins…
f678e52 
…ics:

-  __builtin_riscv_cv_simd_extract_h
-  __builtin_riscv_cv_simd_extract_b
-  __builtin_riscv_cv_simd_extractu_h
-  __builtin_riscv_cv_simd_extractu_b
-  __builtin_riscv_cv_simd_insert_h
-  __builtin_riscv_cv_simd_insert_b
@adeel10x
Copy link
Contributor Author

I've added a couple of comments but otherwise it looks good. Could you also check whether there are Sema tests for this kind of validations? We could do with a test for these

I see there are not many Sema tests so probably the best way to test the errors is to add cases to CodeGen/RISCV/corev-intrinsics/simd.c where we check that the error message is generated.

Hi @paolo,
I have one confusion. If I add failing cases to simd.c, wouldn't that cause the file to not compile causing all other cases to fail?
So, shouldn't I create a new file e.g: simd-error.c, and test for erroneous code there? Please let me know if I'm missing something here.
Thanks

@PaoloS02
Copy link
Contributor

Legit question. You'll need a separate test like clang/test/CodeGen/RISCV/rvv_errors.c to just check the ranges., so yes, a simd-errors.c test sounds like a good idea.

@PaoloS02
Copy link
Contributor

On a separate note, I see that the diffs contain a lot of indentation changes. Has some formatting happened? Would it be possible to reload a patch with just your changes?

@adeel10x
Copy link
Contributor Author

On a separate note, I see that the diffs contain a lot of indentation changes. Has some formatting happened? Would it be possible to reload a patch with just your changes?

Yeah, sure. I ran clang-format by mistake (force of habit). I'll revert it.

@adeel10x
Revert "clang formatted"
93b1c0a 
This reverts commit 1c372bc.
@adeel10x
[RISCV][Clang] Added tests to check the immediate operand range for t…
5e73467 
…he following intrinsics:

    -  __builtin_riscv_cv_simd_extract_h
    -  __builtin_riscv_cv_simd_extract_b
    -  __builtin_riscv_cv_simd_extractu_h
    -  __builtin_riscv_cv_simd_extractu_b
    -  __builtin_riscv_cv_simd_insert_h
    -  __builtin_riscv_cv_simd_insert_b
@adeel10x
Copy link
Contributor Author

Legit question. You'll need a separate test like clang/test/CodeGen/RISCV/rvv_errors.c to just check the ranges., so yes, a simd-errors.c test sounds like a good idea.

I have added the tests.

@adeel10x
Copy link
Contributor Author

Hi @PaoloS02 ,
There are some tests for __builtin_riscv_cv_simd_extract* in simd.c. These tests will fail after the recent changes as they are using values beyond the allowed range. Should I remove __builtin_riscv_cv_simd_extract* tests that are using a negative value for the immediate operand from simd.c, as I've already added such tests in simd-errors.c.

Also, I think I should adjust the value of the immediate operand for those __builtin_riscv_cv_simd_extract* tests in simd.c that are using positive immediate value but the value is out of valid range.

@PaoloS02
Copy link
Contributor

Hi @adeel10x sorry for the delay. Yes to all the above. Since we have a separate test to cover the error cases so that we can see whether future changes break the check on the operand all we need in the simd.c test is normal use cases with appropriate operands to check that the selection of the intrinsics works. Thanks for this!

@PaoloS02
Copy link
Contributor

The new tests look good!

@adeel10x
Modified tests for the following builtins:
e6aa6ef 
__builtin_riscv_cv_simd_extract_h
__builtin_riscv_cv_simd_extract_b
__builtin_riscv_cv_simd_extractu_h
__builtin_riscv_cv_simd_extractu_b
@PaoloS02 PaoloS02 merged commit e93f95c into openhwgroup:development Dec 27, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaoloS02 PaoloS02 PaoloS02 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adeel10x @PaoloS02