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

qa_utils token parsing might be insufficient #326

Open
michaelld opened this issue Jan 13, 2020 · 10 comments
Open

qa_utils token parsing might be insufficient #326

michaelld opened this issue Jan 13, 2020 · 10 comments

Comments

@michaelld
Copy link
Contributor

This line < https://github.com/n-west/volk/blob/master/lib/qa_utils.cc#L157 >, according to @ValZapod < #59 (comment) >:

Is currently:

            if(token[0] == 'x' && (token.size() > 1) && (token[1] > '0' || token[1] < '9')) { //it's a multiplier

Consider instead

            if(token[0] == 'x' && (token.size() > 1) && (token[1] >= '0' && token[1] <= '9')) { //it's a multiplier
@michaelld
Copy link
Contributor Author

Indeed LOL! Can we have a multiplier that's '0'?

@michaelld
Copy link
Contributor Author

Does anyone know if we have a place where we describe volk kernel name parsing?

Meaning: Looking at the kernel volk_16i_branch_4_state_8.h, I have 6 tokens (assuming separator _). for volk_32f_x2_dot_prod_16i.h I have 6 tokens as well, but they are clearly somewhat different tokens and the kernels are of course very different. Do we have a file somewhere that describes how to parse these names to make sense of them? I've been poking through the repo & have found nothing obvious just yet ... and it's not like the repo has tons of files in it!

@michaelld
Copy link
Contributor Author

OK so yes Doug says x[0-9]+ for the multiplier. What I'm going for is: what is a multiplier & can it really be 0? Can it be 15? We need to fix that line anyway, so let's do it correctly for the reality of what the multiplier is. Hence my query about kernel naming ...

@michaelld
Copy link
Contributor Author

@ValZapod any thoughts on this Volk issue? care to put together a PR for it?

@michaelld
Copy link
Contributor Author

@ValZapod the token should be "x#", where # is a number in [0-9] ... but I'm not 100% sure if we can have "x0" ... certainly "x[2-5]" since those exist. So ... thinking just leaving the check to be in [0-9] is the way to go, just to fix this to be && rather than || as noted in the issue opening.

@michaelld
Copy link
Contributor Author

certainly "x[2-5]" since those exist

Perfect. Then just change || to &&. And when (if) you need x0 multiplier edit the code.

yup. I like this tweak ... simple, keeps what I think the intent of the code was supposed to be.

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

No branches or pull requests

2 participants
@michaelld and others