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

[slang] Fix PLA task concatenation ascending order #1047

Merged
merged 2 commits into from
Jul 11, 2024

Conversation

likeamahoney
Copy link
Contributor

Added support for elaborating correctness of examples like the one from LRM section 20.16:

module async_array(a1,a2,a3,a4,a5,a6,a7,b1,b2,b3);
  input a1, a2, a3, a4, a5, a6, a7 ;
  output b1, b2, b3;
  logic [1:7] mem[1:3]; // memory declaration for array personality
  logic b1, b2, b3;
  initial begin
      // set up the personality from the file array.dat
     $readmemb("array.dat", mem);
     // set up an asynchronous logic array with the input
     // and output terms expressed as concatenations
     $async$and$array(mem,{a1,a2,a3,a4,a5,a6,a7},{b1,b2,b3});
  end
endmodule

By default, slang sets all concatenation expression bitwidth ranges in descending order. I added an ASTFlag to change this behavior when concatenation is an argument of the PLA task.

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.71%. Comparing base (4221f2f) to head (3b40ce6).
Report is 4 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1047   +/-   ##
=======================================
  Coverage   94.71%   94.71%           
=======================================
  Files         191      191           
  Lines       47656    47667   +11     
=======================================
+ Hits        45136    45147   +11     
  Misses       2520     2520           
Files Coverage Δ
source/ast/builtins/SystemTasks.cpp 91.13% <100.00%> (+0.03%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4221f2f...3b40ce6. Read the comment docs.

@MikePopoloski
Copy link
Owner

This seems like an overly intrusive way of fixing this -- I'd prefer to avoid adding a specialized ASTFlag just for this one case. It seems like it should be fixable in the PLATask handler itself by changing checkArguments to see that it's a concat expression and just not issuing an error in that case.

@@ -725,9 +725,9 @@ class SdfAnnotateTask : public SystemTaskBase {
}
};

class PlaTask : public SystemTaskBase {
class SLANG_EXPORT PlaTask : public SystemTaskBase {
Copy link
Owner

Choose a reason for hiding this comment

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

Adding SLANG_EXPORT here doesn't do anything; this class is not externally visible to downstream users of the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@likeamahoney
Copy link
Contributor Author

This seems like an overly intrusive way of fixing this -- I'd prefer to avoid adding a specialized ASTFlag just for this one case. It seems like it should be fixable in the PLATask handler itself by changing checkArguments to see that it's a concat expression and just not issuing an error in that case.

fixed

@MikePopoloski MikePopoloski merged commit ce12896 into MikePopoloski:master Jul 11, 2024
15 checks passed
JoelSole-Semidyn added a commit to JoelSole-Semidyn/slang that referenced this pull request Jul 12, 2024
…dev/slang-tidy

* 'master' of https://github.com/MikePopoloski/slang:
  Update CHANGELOG.md
  [slang] Fix PLA task concatenation ascending order (MikePopoloski#1047)
  [slang] Fix PATHPULSE limit values (MikePopoloski#1048)
  Handle recursive parameter definitions via hierarchical reference
  Mark fmt lib headers as system headers to suppress warnings
  Bump dependency versions: fmt and pybind11
  [slang] Make string simple type (MikePopoloski#1049)
  Update README.md (MikePopoloski#1046)
  [slang][port] Fix bugs (MikePopoloski#1043)
  Use [[likely]] / [[unlikely]] replace macro SLANG_LIKELY/SLANG_UNLIKELY (MikePopoloski#1038)
  Add multiple slang-tidy checks and minor fixes in existing ones (MikePopoloski#1040)
  chore: update pre-commit hooks (MikePopoloski#1041)
JoelSole-Semidyn pushed a commit to JoelSole-Semidyn/slang that referenced this pull request Jul 15, 2024
JoelSole-Semidyn added a commit to JoelSole-Semidyn/slang that referenced this pull request Jul 15, 2024
…dev/slang-tidy

* 'master' of https://github.com/MikePopoloski/slang:
  Fix a bug in the tidy-check UnusedSensitiveSignal (MikePopoloski#1056)
  Update CHANGELOG.md
  [slang] Fix PLA task concatenation ascending order (MikePopoloski#1047)
  [slang] Fix PATHPULSE limit values (MikePopoloski#1048)
  Handle recursive parameter definitions via hierarchical reference
  Mark fmt lib headers as system headers to suppress warnings
  Bump dependency versions: fmt and pybind11
  [slang] Make string simple type (MikePopoloski#1049)
  Update README.md (MikePopoloski#1046)
  [slang][port] Fix bugs (MikePopoloski#1043)
  Use [[likely]] / [[unlikely]] replace macro SLANG_LIKELY/SLANG_UNLIKELY (MikePopoloski#1038)
  Add multiple slang-tidy checks and minor fixes in existing ones (MikePopoloski#1040)
  chore: update pre-commit hooks (MikePopoloski#1041)
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