Glow: ExecutionEngine::compile() modifies Function

[geoffberry]

If a backend implements transformPreLowering() or transformPostLowering(), the Function parameter to ExecutionEngine::compile() is modified in place, so subsequent calls to ExecutionEngine::compile() with the same function will get the transformed Function as input. This can lead to problems if the function is compiled with different backends or if the backend transforms aren't prepared to handle the backend-specific nodes they may produce as input.

[jfix71]

@geoffberry Thanks for opening this issue. What use case are you running into here where this is an issue? In general I think if you want to load a Function once and then compile it for multiple backends then you should clone it before compiling it.

E.g. when we do profiling and quantization we clone the function and then modify the clone, so if you profile on a different backend than you quantize with, then there are separate functions.

[geoffberry]

@jfix71 Thanks for the prompt reply.

The use case I'm running into is e.g. the OperatorTest Squeeze test. Subsequent calls to EE_.compile() will see a Function that has had previous transformPreLowering()/transformPostLowering() applied to it. If those transforms introduce new backend-specific nodes that don't normally appear in the input then the transform code now has to handle these backend-specific nodes as input.

[jfix71]

Got it. For Squeeze in OperatorTest it seems to me we shouldn't be reusing F_ -- we should instead be creating a new Function inside the Module for each new call to compile, especially because the nodes in each test that are added to F_ are independent other than reading from inputs. Alternatively those three tests all currently inside one could be separated into three actual individual tests with different Module, EE, F_, etc.

Assuming the main problem here is just in unit tests, I think that's the best path forward here. If there are other cases in which this is a problem, we'd be happy to discuss what the problem is and what makes sense as a solution.

[geoffberry]

I'm fine with this being addressed in the unit tests (that's the only place I've seen it come up).

[jfix71]

@geoffberry Would you like to work on the solution? If not, it would be great if you could list all unit tests you've encountered the issue.

[geoffberry]

@jfix71 Unfortunately, I can't currently sign the CLA so I can't post a patch.
I've only run into this so far in two tests: OperatorTest:Squeeze and OperatorTest:testQuantizedBatchAdd, but my tree is a bit behind.

Could we potentially catch cases like this by adding a bit to the Function that records whether it has been compiled and raise an error if a compiled function is compiled again?

[jfix71]

Unfortunately, I can't currently sign the CLA so I can't post a patch.
I've only run into this so far in two tests: OperatorTest:Squeeze and OperatorTest:testQuantizedBatchAdd, but my tree is a bit behind.

@geoffberry No worries, thanks for the info.

Could we potentially catch cases like this by adding a bit to the Function that records whether it has been compiled and raise an error if a compiled function is compiled again?

Yeah I think this probably makes sense. There are other potential bugs here too in addition to the issue of backend-specific nodes having been added, e.g. because the graph optimizer transforms the graph based on knowing all the users of each node etc.

[jfix71]

@gcatron Similar to what we talked about offline RE: compiling a function multiple times. Your PR should fix this one.