Skip to content

new v93k loop PR #158

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

new v93k loop PR #158

wants to merge 4 commits into from

Conversation

info-rchitect
Copy link
Member

@info-rchitect info-rchitect commented May 6, 2020

Open questions:

  • ATP loop method accepts start, stop, increment, and optional flag. If the user supplies the optional flag should the stop argument be ignored?
  • If the optional flag argument is supplied, should the flag be set to the stop value on the line before the for loop?

thx

@info-rchitect info-rchitect requested a review from a team May 6, 2020 20:36
@info-rchitect info-rchitect changed the title Changes as suggested by info-architect in his PR new v93k loop PR May 6, 2020
@@ -3764,6 +3794,16 @@ test_flow
else
{
}
for @index = 0; @index < @loop ; @index = @index + 1; do
test_number_loop_increment = 0
Copy link
Member

Choose a reason for hiding this comment

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

What's this test_number_loop_increment ?

Copy link
Member

Choose a reason for hiding this comment

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

@info-rchitect is this something that just needs to be removed from here?

@@ -1751,5 +1765,15 @@ flow FLOW_CONTROL {
else
{
}
for @index = 0; @index < @loop ; @index = @index + 1; do
Copy link
Member

Choose a reason for hiding this comment

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

I doubt this is valid smt8, its fine to not support it on SMT8 yet, but in that case either output nothing and emit a warning, or else raise an error rather than generating garbage.

Copy link
Member

Choose a reason for hiding this comment

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

@info-rchitect can you look at this? adding it in a tester.smt7? check should be okay

@info-rchitect
Copy link
Member Author

@rlaj Can you please review this PR as it uses the ATP loop method you created.

if index_flag.is_a?(String)
line "for @#{index_flag} = #{loop_start}; @#{index_flag} < #{loop_end} ; @#{index_flag} = @#{index_flag} + #{loop_inc}; do"
else
line "for @index = #{loop_start}; @index < #{loop_end} ; @index = @index + #{loop_inc}; do"
Copy link
Member

@ginty ginty May 8, 2020

Choose a reason for hiding this comment

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

I think you need to generate something more unique than just @index, e.g. consider the case of nested loops.
If you look around in here there should be other examples of generating unique ids.

Copy link
Member Author

@info-rchitect info-rchitect May 8, 2020

Choose a reason for hiding this comment

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

@ginty Is that the responsibility of Origen? Seems like that should be up to the user similar to how one would use different variables in a C for loop.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is the kind of detail that a framework should take care of

Copy link
Member

Choose a reason for hiding this comment

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

Also consider the Origen API is trying to be as ATE-agnostic as possible. If a user only wants to loop X times, then they should be able to express that without worrying about flow vars. Such things may not even make sense on other platforms.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also consider the Origen API is trying to be as ATE-agnostic as possible. If a user only wants to loop X times, then they should be able to express that without worrying about flow vars. Such things may not even make sense on other platforms.

The current ATP loop API allows the user to pass an integer or a string representing a flow variable for the to option. Wouldn't we just handle that in each of the tester generators? If a particular generator does not use an option available in the ATP flow method then it errors and instructs the user.

Copy link
Member

Choose a reason for hiding this comment

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

This is the way Origen works man, with the current loop API I would and should expect this to just work:

loop from: 1, to: dut.number_of_x, step: 1 do
    loop from: 1, to: dut.number_of_y, step: 1 do
      func :some_test
    end
end

If the v93k needs a flow var to be defined to enable that to work, then its Origen's responsibility to create one (if the user doesn't care about what it's called), and make sure what it is creating will work in cases like the above.

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.

3 participants