Pulse Compiler SetSequence pass
#11980
Conversation
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
|
One or more of the the following people are requested to review this:
|
TsafrirA
added
mod: pulse
| pm.append(SetSequence()) | ||
| return pm | ||
|
|
||
| @named_data(*ddt_named_data) |
There was a problem hiding this comment.
I tested everything against all relevant alignment types. It's kind of an overkill, with the logic located at a parent abstract class.
There was a problem hiding this comment.
Thanks @TsafrirA . The sequence logic looks good to me. My main concern is about implementation of sequence pass, and I think the sequence logic must be implemented inside the pass, rather than delegating it to the alignment object.
Here is another question. When we talk to a hardware, I think they request mixed frames. However, in the current implementation, broadcast is not really implemented. Instead, the sequence pass just create edges to a common node to broadcast. Do you want to write a separate pass for broadcast? We can also do this broadcast inside the sequencing pass with slight modification.
| self._analyze_mixed_frames_in_sequence(passmanager_ir) | ||
| self.property_set["mixed_frames_mapping"] = self.mixed_frames_mapping |
There was a problem hiding this comment.
| self._analyze_mixed_frames_in_sequence(passmanager_ir) | |
| self.property_set["mixed_frames_mapping"] = self.mixed_frames_mapping | |
| mixed_frames_mapping = defaultdict(set) | |
| self._analyze_mixed_frames_in_sequence(passmanager_ir, mixed_frames_mapping) | |
| self.property_set["mixed_frames_mapping"] = mixed_frames_mapping |
Can we implement the logic like this? Statefull pass sometimes causes edge cases. For example, in the current implementation some unexpected things could happen if you run the same pass manager instance twice for different inputs.
There was a problem hiding this comment.
The issue of re-running a pass is something that bothered me too. Do we override existing data? Raise an error? There's a "global" decision to be made here, because it's applicable to all passes.
There was a problem hiding this comment.
For the current pass, I modified it per your suggestion.
There was a problem hiding this comment.
There is definitely a demand for rerunning the pass manager, for example #11784. I prefer pass that doesn't have state, except for the user provided arguments.
There was a problem hiding this comment.
In that case, should we remove all edges from the sequence before sequencing?
|
|
||
| def __init__(self): | ||
| """Create new MapMixedFrames pass""" | ||
| super().__init__(target=None) |
There was a problem hiding this comment.
This violates typehint (though type is not important in Python). Do you want to remove Target from the base passes?
There was a problem hiding this comment.
As I commented on the original compiler PR, we don't always need Target. I think we can make it optional.
Summary
A
SetSequencepass is introduced. The pass transforms Pulse'sSequenceIRby setting the sequence of elements (adding edges to the graph). A second analysis pass is introduced -MapMixedFrame- which is needed for sequencing of parallel alignments.Details and comments
When
SequenceIRobject is populated, itssequenceproperty includes only nodes and no edges. Setting the sequence (adding the edges to the graph) depends on the alignment type of the object. The passSetSequenceadds the edges to the graph according to the alignment type.For parallel alignment, a mapping of
PulseTargetandFrametoMixedFrames is needed. For this purpose, an analysis passMapMixedFrameis also added. The pass recurses through the IR, and identifies creates the mapping.