[HW] Module port accessors and modifiers bugs

[youngar]

A long time ago, HW module op would store its inputs and outputs using a function type. This lacked the ability to intermix input and output ports the way verilog allows, and so we overhauled the system. We added a ModuleType which holds an array of all ports, storing the name, type, and direction of each. It seems that different parts of our code have not been updated properly with the new system.

---

In the HWModuleOpInterface, the setInputNames makes an assumption that the port names of inputs are stored before all outputs. When you update the input port names, it will replace replace all the existing input port names, but then effectively partially sort by port direction before commiting the names.

https://github.com/llvm/circt/blob/a3e993cb387a08e686b0df1d7f5b599bca0b3751/include/circt/Dialect/HW/HWOpInterfaces.td#L135-L140

This issue can be found in setInputNames, setOutputNames, setAllInputAttrs, setAllOutputAttrs, setInputLocs, setOutputLocs.

---

modifyModuleArgs is used to insert and delete ports from modules. Due to historical reasons, it treats input and output ports differently. I believe that this function will successfully modify the input and output ports as requested, but then follow up by sorting them by direction. I don't think this is on purpose, and would argue it would make sense to retain the original port order. I.e. the input and output indices should be changed in to a global port index. An ever better API here would be to change this function so that it used a single list of port modifications for both inputs and outputs.

https://github.com/llvm/circt/blob/a3e993cb387a08e686b0df1d7f5b599bca0b3751/lib/Dialect/HW/HWOps.cpp#L582

---

HWInstanceOp getPortIdForInputId and getPortIdForOutputId are wrong, and ~should just forward the question to the ModuleType~ the InstanceOp does not have enough information to properly implement these methods as is.

https://github.com/llvm/circt/blob/a3e993cb387a08e686b0df1d7f5b599bca0b3751/include/circt/Dialect/HW/HWStructure.td#L458-L462

https://github.com/llvm/circt/blob/a3e993cb387a08e686b0df1d7f5b599bca0b3751/lib/Dialect/HW/HWTypes.cpp#L825

---

This is not a bug, but this code could be faster. Using getPortIdForInputId and getPortIdForOutputId involves a serial walk through all ports. It would be faster to just walk all ports, filtering out the unneeded directions. Same issue in getInputLocs, getInputLocsAttr, getOutputLocs, getOutputLocsAttr.

https://github.com/llvm/circt/blob/a3e993cb387a08e686b0df1d7f5b599bca0b3751/include/circt/Dialect/HW/HWOpInterfaces.td#L178-L200

---

Audit the use of HWMutableModuleLike. Is it still needed, and can the functionality be collapsed in to the regular HWModuleLike?

appendOutputs should probably be implemented the way other methods are in this file, i.e. by lightly wrapping modifyPorts. It does not look like its implemented properly for all module types as is:

void HWModuleExternOp::appendOutputs(
    ArrayRef<std::pair<StringAttr, Value>> outputs) {}

void HWModuleGeneratedOp::appendOutputs(
    ArrayRef<std::pair<StringAttr, Value>> outputs) {}

[darthscsi]

| HWInstanceOp getPortIdForInputId and getPortIdForOutputId are wrong, and should just forward the question to the ModuleType

This breaks hw.module parallelization.

[youngar]

I forgot that InstanceOp doesn't hold a ModuleType. I guess there is no way to implement these functions as is, and so they should just be deleted.

[darthscsi]

The intention is to get rid of getInput* and getOutput*. Nothing in HW is suppose to use them, everything there should be using port based versions. Thus these aren't suppose to impact performance for migrated dialects.

[darthscsi]

Also referenced (commit message wrong) in 9e0c1696f3caef4059c65774ad6b8efee91d9d9e