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

[Bounty $20] Two inline assignments in the same statement produce incorrect opencl code. #99

Open
freemo opened this issue Apr 20, 2018 · 3 comments
Labels
bounty $$$ Cash reward! bug Fix something that is broken

Comments

@freemo
Copy link
Member

freemo commented Apr 20, 2018

The problem arrises from java statements like this:

foobar = foo = bar

which incorrectly generates this in opencl instead:

result = int assignMe = value;

This bug is demonstrated in this unit test:
https://github.com/Syncleus/aparapi/blob/master/src/test/java/com/aparapi/codegen/test/FirstAssignInExpression2Test.java

when run it produces the following exception:

rhs:1> 
lhs:14< int result = 0;
lhs:15< if (value==value){
lhs:16< result = int assignMe = value;
lhs:17< } else {
lhs:18< int assignMe = 1;
lhs:19< result = 2;
lhs:20< }
lhs:21< result++;
lhs:22< assignMe++;
lhs:23< return;
lhs:24< }
rhs:15> int result=0;
rhs:16> int assignMe=0;
rhs:17> if (true){
rhs:18> result = assignMe = value;
rhs:19> }else{
rhs:20> assignMe =1;
rhs:21> result=2;
lhs:26> result++;
lhs:26> return;
lhs:26> }
lhs:26> }
lhs:26> 
---com.aparapi.codegen.test.FirstAssignInExpression2------------------------------------------------------------------------------
Expected {
typedef struct This_s{

 int passid;
 }This;
 int get_pass_id(This *this){
 return this->passid;
 }
 __kernel void run(
 int passid
 ){
 This thisStruct;
 This* this=&thisStruct;
 this->passid = passid;
 {
 int value = 1;
 int result=0;
 int assignMe=0;
 if (true){
 result = assignMe = value;
 }else{
 assignMe =1;
 result=2;
 }
 result++;
 return;
 }
 }
 
}Actual
{typedef struct This_s{
   int passid;
}This;
int get_pass_id(This *this){
   return this->passid;
}
__kernel void run(
   int passid
){
   This thisStruct;
   This* this=&thisStruct;
   this->passid = passid;
   {
      int value = 1;
      int result = 0;
      if (value==value){
         result = int assignMe = value;
      } else {
         int assignMe = 1;
         result = 2;
      }
      result++;
      assignMe++;
      return;
   }
}

}
------------------------------------------------------------------------------------------------------

org.junit.ComparisonFailure: FirstAssignInExpression2  <Click to see difference>


	at org.junit.Assert.assertEquals(Assert.java:115)
	at com.aparapi.codegen.CodeGenJUnitBase.test(CodeGenJUnitBase.java:116)
	at com.aparapi.codegen.test.FirstAssignInExpression2Test.FirstAssignInExpression2TestWorksWithCaching(FirstAssignInExpression2Test.java:61)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)

@freemo freemo added bug Fix something that is broken bounty $$$ Cash reward! labels Apr 20, 2018
@freemo freemo changed the title Two inline assignments in the same statement produce incorrect opencl code. [Bounty $20] Two inline assignments in the same statement produce incorrect opencl code. Apr 20, 2018
@toonijn
Copy link
Contributor

toonijn commented Jul 10, 2019

Upon investigating: the problem isn't really

int b = 0;
int a = b = 0;

the problem is

int b;
int a = b = 0;

The statement int b; doesn't really produce byte code. So the first time it is seen it can't be declared.
It has exactly the same problem as #97 . Throwing an error would work, but less than perfect. Maybe I'll try to fix it, but it won't be easy: the BlockWriter seems not able to insert a line before the current one.

@toonijn
Copy link
Contributor

toonijn commented Jul 10, 2019

It maybe is beneficial to just throw the concept of multi_assign out. In essence, it's just a simple assign and one (or more) inline assigns.

@toonijn
Copy link
Contributor

toonijn commented Jul 12, 2019

Because declarations without assignment (e.g. int a;) are not saved in the byte code or local variable table, it is very hard to discover what the scope of a variable is. I suspect most decompilers will guess the broadest scope possible.

I suggest rethinking how to work with variables. One possibility is to ignore the local variable table and have just variables like: i1, d2, a0... with i1 an int in variable with index 1, a0 would be the reference in variable with index 0 (in most cases 'this'). These names are closer to the bytecode and scope independent. Using this method will fix at least following issues: #97 #99 #147

A big disadvantage is that a lot of code should be revised, and most (all) of the codegen tests will have to be rewritten.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty $$$ Cash reward! bug Fix something that is broken
Projects
None yet
Development

No branches or pull requests

2 participants