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

Assignments to components of fields within a struct array are optimized away by the runtime optimizer #834

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

marsupial
Copy link
Contributor

@marsupial marsupial commented Dec 18, 2017

Description

The following code leaves c[0].rgb[0] in an undefined state when the runtime optimizer is used:

struct custom { color rgb; };


custom c[1];
c[0].rgb[0] = 123;
printf("rgb: %g\n", c[0].rgb);
printf("rgb: %g\n", c[0].rgb[0]);

Patch add ability to have a test pass who's diff operation fails.
Fixes nothing related to the bug, but adds a test showing the behavior.

Checklist:

  • I have read the contribution guidelines.
  • I have previously submitted a Contributor License Agreement.
  • I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

@lgritz
Copy link
Collaborator

lgritz commented Dec 19, 2017

Are you already working on a solution, or just giving us the test and assuming that I'll find the fix?

I'm truly not saying that in an accusatory way... I'm very happy to jump right on fixing this, but I don't want to do the work if it's a waste because you already are in the middle of it but haven't posted a PR yet.

@marsupial
Copy link
Contributor Author

I assume nothing, so have at it!
Was trying to fix it in olsc, but didn't get far and realized the runtime optimizer would probably still have to be addressed in case one loaded an oso compiled with an older version.

@lgritz
Copy link
Collaborator

lgritz commented Dec 20, 2017

Yeah, I'm happy to take a stab at this.

@marsupial
Copy link
Contributor Author

Good luck. In case it not too obvious already, it looked to be generating tmps for every access and not getting the aliasing or r/w boundary right. Which cause the runtime optimizer to think is can be elided as a useless write.

c[0].rgb[0] = 123; ->

const	int	$const1	0		%read{0,17} %write{2147483647,-1}
temp	color	$tmp1	%read{2147483647,-1} %write{0,0}
const	color	$const2	1 2 3		%read{1,1} %write{2147483647,-1}
temp	color	$tmp2	%read{3,3} %write{2,2}
temp	float	$tmp3	%read{4,4} %write{3,3}
temp	float	$tmp4	%read{6,6} %write{4,4}
temp	color	$tmp5	%read{2147483647,-1} %write{5,6}
code ___main___
# test.osl:10
#     c[0].rgb = color(1,2,3);
	aref		$tmp1 c.rgb $const1 	%filename{"test.osl"} %line{10} %argrw{"wrr"}
	aassign		c.rgb $const1 $const2 	%argrw{"wrr"}
# test.osl:13
#     c[0].rgb[0] = -c[0].rgb[0];
	aref		$tmp2 c.rgb $const1 	%line{13} %argrw{"wrr"}
	compref		$tmp3 $tmp2 $const1 	%argrw{"wrr"}
	neg		$tmp4 $tmp3 	%argrw{"wr"}
	aref		$tmp5 c.rgb $const1 	%argrw{"wrr"}
	compassign	$tmp5 $const1 $tmp4 	%argrw{"wrr"}

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.

2 participants