KULeuven-MICAS / zigzag

HW Architecture-Mapping Design Space Exploration Framework for Deep Learning Accelerators
https://kuleuven-micas.github.io/zigzag/
MIT License
104 stars 36 forks source link

Fix SearchUnusedMemoryStage and RemoveUnusedMemoryStage #57

Closed JiacongSun closed 3 months ago

JiacongSun commented 4 months ago

Fix SearchUnusedMemoryStage and RemoveUnusedMemoryStage

JiacongSun commented 4 months ago

The onnx parser will generate broken graphs (for such as mobilenetv2.onnx, resnet18.onnx) and need to be fixed. The visualization function below is enabled for debugging. https://github.com/KULeuven-MICAS/zigzag/blob/4e923c3b0c451de2b4afaf5f874664948d05fe0a/zigzag/parser/onnx/ONNXModelParser.py#L97

RobinGeens commented 3 months ago

@asyms @JiacongSun As discussed, we would also like to rename these stages to something that describes better what it does. Are there any suggestions? What about RemoveLayerTransferOverhead ?

JiacongSun commented 3 months ago

@asyms @JiacongSun As discussed, we would also like to rename these stages to something that describes better what it does. Are there any suggestions? What about RemoveLayerTransferOverhead ?

I feel RemoveLayerTransferOverhead or RemoveLayerTransferOverheadStage is less intuitive compared to the original RemoveUnusedMemoryStage. I prefer to keep the original name or stress more on the point of data locality (such as MemoryDataLocalityStage?)

RobinGeens commented 3 months ago

@asyms @JiacongSun As discussed, we would also like to rename these stages to something that describes better what it does. Are there any suggestions? What about RemoveLayerTransferOverhead ?

I feel RemoveLayerTransferOverhead or RemoveLayerTransferOverheadStage is less intuitive compared to the original RemoveUnusedMemoryStage. I prefer to keep the original name or stress more on the point of data locality (such as MemoryDataLocalityStage?)

What about ExploitInterLayerDataLocalityStage ?

JiacongSun commented 3 months ago

@asyms @JiacongSun As discussed, we would also like to rename these stages to something that describes better what it does. Are there any suggestions? What about RemoveLayerTransferOverhead ?

I feel RemoveLayerTransferOverhead or RemoveLayerTransferOverheadStage is less intuitive compared to the original RemoveUnusedMemoryStage. I prefer to keep the original name or stress more on the point of data locality (such as MemoryDataLocalityStage?)

What about ExploitInterLayerDataLocalityStage ?

Good for me! Also rename the previous stage SearchUnsedMemoryStage to SearchInterLayerDataLocalityStage for consistency?

RobinGeens commented 3 months ago

@asyms @JiacongSun As discussed, we would also like to rename these stages to something that describes better what it does. Are there any suggestions? What about RemoveLayerTransferOverhead ?

I feel RemoveLayerTransferOverhead or RemoveLayerTransferOverheadStage is less intuitive compared to the original RemoveUnusedMemoryStage. I prefer to keep the original name or stress more on the point of data locality (such as MemoryDataLocalityStage?)

What about ExploitInterLayerDataLocalityStage ?

Good for me! Also rename the previous stage SearchUnsedMemoryStage to SearchInterLayerDataLocalityStage for consistency?

I would prefer to merge the two stages, since they are completely dependent anyway. This would also solve the naming issue

JiacongSun commented 3 months ago

@asyms @JiacongSun As discussed, we would also like to rename these stages to something that describes better what it does. Are there any suggestions? What about RemoveLayerTransferOverhead ?

I feel RemoveLayerTransferOverhead or RemoveLayerTransferOverheadStage is less intuitive compared to the original RemoveUnusedMemoryStage. I prefer to keep the original name or stress more on the point of data locality (such as MemoryDataLocalityStage?)

What about ExploitInterLayerDataLocalityStage ?

Good for me! Also rename the previous stage SearchUnsedMemoryStage to SearchInterLayerDataLocalityStage for consistency?

I would prefer to merge the two stages, since they are completely dependent anyway. This would also solve the naming issue

OK! That would be great!

asyms commented 3 months ago

I thought the two stages required to be inserted at different points in the list of all stages? How do we plan on merging the two? I see two possibilities I would like your feedback on:

  1. Provide an additional flag from the first call to the second and adjust the stage behavior accordingly, but still keep it twice in the list of stages
  2. Call it once (where RemoveUnusedMemoryStage is now), where we do both. This would require the entire workload graph to be passed to the stage, which would be used in the initialization to find the required memory levels?