Skip to content

Conversation

@vyast-softserveinc
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@pavlo-hilei pavlo-hilei left a comment

Choose a reason for hiding this comment

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

This fixes warnings, but does it fix an underlying issue? Please check comments

case BinOp::Ge:
secondMeta.condition.op = BinOp::Gt;
break;
if (secondMeta.condition.op == BinOp::Le) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that a correct behavior that LessEqual translates to LessThan?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that okay that we skip other BinOp cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is how it was. I would imagine that there could be any kind of Binary/Unary operation for example:

struct MyInfo { int sizes[3] };
....
@kernel void func(..., MyInfo* ptr) {
    @outer for(int i =0 ; i <  *ptr.sizes[0]; i += *ptr.sizes[2]) {
       ...
     }
}

As far as I can see we just pass it as is

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that a correct behavior that LessEqual translates to LessThan?
Yes. That's correct.

if (sz.value_or(1024) > 0) {
if (firstMeta.inc.val.empty()) {
firstMeta.inc.val = loopInfo.tileSize;
switch (firstMeta.inc.op.uo) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

But we still don't handle UnOp::Other case, which will leave firstMeta.inc.op.bo uninitialized

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would ask the author of this code what should happen

Copy link
Collaborator

@kchabSS kchabSS May 27, 2024

Choose a reason for hiding this comment

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

the ::Other were originally there as default values.
In case OP cannot be properly detected (or there is no increment/decrement at all), the default Other is set.
If the Other is detected at the end of loop parsing, an error must be raised.
That is how it was originally written for launcher PoC.

@vyast-softserveinc
Copy link
Collaborator Author

Ok in general I can rename other as unsupported and then provide an error. Have to add the tests and check under the original occa.

Copy link
Collaborator

@kchabSS kchabSS left a comment

Choose a reason for hiding this comment

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

The changes look OK.

case BinOp::Ge:
secondMeta.condition.op = BinOp::Gt;
break;
if (secondMeta.condition.op == BinOp::Le) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that a correct behavior that LessEqual translates to LessThan?
Yes. That's correct.

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.

5 participants