phate / jlm

GNU Lesser General Public License v2.1
51 stars 13 forks source link

Add support for calls to invariant value redirection #503

Closed phate closed 3 months ago

phate commented 3 months ago

This PR does the following:

  1. Adds support for non-recursive direct calls to invariant value redirection
  2. Adds support for memory state operation nodes to invariant value redirection
  3. Adds helper methods to call node class to retrieve respective memory state operation nodes
  4. Adds helper methods to lambda node class to retrieve respective memory state operation nodes
  5. Some minor refactoring of the invariant value redirection tests

The above improvements handle the common cases. I marked the places in the code where we can still do improvements with FIXMEs, but these should not be very common and can be done another time.

caleridas commented 3 months ago

Not terribly much to say, looks good as to what it does.

There is one aspect that makes me think, namely that this makes a number of assumptions regarding the particular structure inside the function body (entry memory split node...) where I am wondering how this will go when generalizing the function type. I still have the intent of providing a generic lambda in rvsdg of from which this should ideally specialize, and slightly wondering if the transform then also still applies generically.

phate commented 3 months ago

Fixed the typo in the documentation and merged master into it.

phate commented 3 months ago

@caleridas Regarding your lambda comment. I do not know the specifics obviously, but it should be possible to derive a concrete LLVM lambda with an IOStateType and MemoryStateType as last argument/result from this general lambda, otherwise this does not make much sense, no? I am currently not really sure where you see the problem. Let's discuss it once we are there.

caleridas commented 3 months ago

@caleridas Regarding your lambda comment. I do not know the specifics obviously, but it should be possible to derive a concrete LLVM lambda with an IOStateType and MemoryStateType as last argument/result from this general lambda, otherwise this does not make much sense, no? I am currently not really sure where you see the problem. Let's discuss it once we are there.

There is no real "problem", was just wondering whether you had spent thoughts on that. Yes FunctionType should be derivable in that form, it was more whether the transformation you have here would also apply to the generic form.

phate commented 3 months ago

@caleridas Yes, this problem arose a few times by now. Some transformations, such as dead node elimination or invariant value redirection, are more general than just for the LLVM dialect of jlm. This problem already came up with the HLS dialect, which has its own version of dead node elimination or invariant value redirection, as they needed to take care of their "theta node" (and the LLVM nodes they depend on). We currently have no "framework" in the RVSDG library that could be used by the downstream dependencies (HLS or LLVM dialect) to build such extensible transformations. Something needs to happen in that direction, but I have not had the capacity to give it too much thought yet. This is certainly not the right forum to have this discussion. We should open a thread in the discussion section for it.