[NFC] change ModulePortInfo to store in a single array and get rid of redundant APIs. This is preparation for not splitting inputs and outputs. #5680

darthscsi · 2023-07-25T20:48:36Z

No description provided.

… redundant APIs. This is preparation for not splitting inputs and outputs.

teqdruid · 2023-07-25T20:54:33Z

include/circt/Dialect/HW/HWOpInterfaces.h

-      else
-        inputs.push_back(port);
-    }
+  explicit ModulePortInfo(ArrayRef<PortInfo> inputs,


I thought part of the reason to switch to ModuleType (name?) which is presumably the shift which this preparing for is to preserve module port ordering amongst inputs/outputs? If so, isn't sorting them here antithetical to that?

This is just a storage format change for the current class, so it is preserving existing behavior. Getting everything going through some iterators is the main point.

If it's to facilitate efficient computation of at_input, based on a quick skim of these changes, I don't see any uses of at_input or at_output.

shit... didn't see your comment. fucking gh.

This is just a storage format change for the current class, so it is preserving existing behavior. Getting everything going through some iterators is the main point.

I'm not noticing any places where inputs or outputs are randomly accessed... all the places you modified work through iterators, afaict.

though preserving existing behavior for the sake of incrementalism is reasonable.

|I'm not noticing any places where inputs or outputs are randomly accessed

There are a couple places where things are deleted. Annoying,.

teqdruid

Thanks for taking this on! Moving to a non-ordered list of ports is future work, yes? It could break a bunch of stuff, in particular tests I'd imagine.

teqdruid · 2023-07-25T21:14:28Z

lib/Conversion/HandshakeToHW/HandshakeToHW.cpp

@@ -307,7 +307,7 @@ static LogicalResult convertExtMemoryOps(HWModuleOp mod) {
  OpBuilder b(mod);

  auto getMemoryIOInfo = [&](Location loc, Twine portName, unsigned argIdx,
-                             ArrayRef<hw::PortInfo> info,
+                             ArrayRef<hw::PortInfo *> info,


Why was it necessary to move to a pointer here?

consequence of ArrayRef from a serquential container's iterators.

teqdruid · 2023-07-25T21:14:59Z

lib/Dialect/Calyx/CalyxOps.cpp

@@ -1674,8 +1674,8 @@ verifyPrimitiveOpType(PrimitiveOp instance,
           << "'.";

  // Verify the instance result ports with those of its referenced component.
-  SmallVector<hw::PortInfo> primitivePorts = referencedPrimitive.getAllPorts();
-  size_t numPorts = primitivePorts.size();
+  auto primitivePorts = getModulePortInfo(referencedPrimitive);


nit: plz don't use auto here.

teqdruid · 2023-07-25T21:16:37Z

lib/Dialect/Calyx/Export/CalyxEmitter.cpp

        os << bitWidth.getName().str();
      } else {
        unsigned int bitWidth = port.type.getIntOrFloatBitWidth();
        os << bitWidth;
      }

-      if (i + 1 < e)
+      if (i + 1 < std::distance(ports.begin(), ports.end()))


This seems less efficient. Perhaps calculate e outside of the loop?

It's just a subtract, and probably is hoisted, but I moved it.

Wouldn't surprise me if the compiler wasn't able to do either optimization and defaulted to iterating through with a counter (which would be necessary if it were a linked list). I don't think smallvector.begin() returns a random_access_iterator...

but whatever.

no, vector's model random_access_iterator whose specialized distance is arithmatic. It's not accidental or guessing that it's a subtration.

teqdruid · 2023-07-25T21:17:39Z

lib/Dialect/ESI/Passes/ESILowerTypes.cpp

@@ -81,8 +81,7 @@ void ESILowerTypesPass::runOnOperation() {
          auto isWindowPort = [](hw::PortInfo p) {
            return hw::type_isa<WindowType>(p.type);
          };
-          return !(llvm::any_of(mod.getPorts().inputs, isWindowPort) ||
-                   llvm::any_of(mod.getPorts().outputs, isWindowPort));
+          return !(llvm::any_of(mod.getPorts(), isWindowPort));


Nice! Good demonstration of how this is useful.

teqdruid · 2023-07-25T21:18:52Z

lib/Dialect/HW/HWOps.cpp

  for (auto [i, barg] : llvm::enumerate(bodyRegion.getArguments())) {
-    inputIdx[info.inputs[i].name.str()] = i;
+    inputIdx[info.at(i).name.str()] = i;


This should probably be at_input to be not so reliant on the internal ordering.

Or even better, switch to using an iterator.

teqdruid · 2023-07-25T21:21:54Z

lib/Dialect/Handshake/Transforms/LowerExtmemToHW.cpp

-  for (auto i : llvm::reverse(argReplacementsIdxs))
-    wrapperModPortInfo.inputs.erase(wrapperModPortInfo.inputs.begin() + i);
+  for (auto i : llvm::reverse(argReplacementsIdxs)) {
+    wrapperModPortInfo.ports.erase(wrapperModPortInfo.begin_input() + i);


it's generally bad practice to rely on code outside of a class to make necessary modifications to synchronized internal datastructures.

Indeed. I was trying to avoid having mutation options at all, but a few places were doing things like this.

teqdruid · 2023-07-25T21:24:08Z

lib/Dialect/MSFT/Transforms/PassCommon.cpp

@@ -54,7 +54,7 @@ StringRef circt::msft::getValueName(Value v, const SymbolCache &syms,
  if (auto blockArg = v.dyn_cast<BlockArgument>()) {
    auto portInfo =
        getModulePortInfo(blockArg.getOwner()->getParent()->getParentOp());
-    return portInfo.inputs[blockArg.getArgNumber()].getName();
+    return (portInfo.begin_input() + blockArg.getArgNumber())->getName();


Yeah, this is another case where at_input is the more efficient way...

It didn't exist yet when I wrote that. But it isn't actually anymore efficient, just more concise.

ahh... yeah. I was thinking that if ports allowed mixed order inputs/outputs the input iterator would have to iterate through from the beginning. So this'll be problematic perf in the future. Not relevant for now.

teqdruid · 2023-07-25T21:25:24Z

include/circt/Dialect/HW/HWOpInterfaces.h

+  const PortInfo &at_output(size_t idx) const { return ports[idx + numInputs]; }
+
+  /// This contains a list of all ports.  Input first.
+  SmallVector<PortInfo> ports;


Why are the member variables public? Since they must be kept in sync, it's probably better to keep them private.

there were a few places that were modifying things directly, but I've issolated those.

I'd be supportive of a completely immutable ModulePortInfo and requiring anything doing mutation to have a local mutated list of ports and recreate the ModulePortInfo from that.

darthscsi added 3 commits July 25, 2023 00:11

[NFC] unify ModulePort and PortInfo

ba5b817

missed one

0795926

[NFC] change ModulePortInfo to store in a single array and get rid of…

ca1969c

… redundant APIs. This is preparation for not splitting inputs and outputs.

darthscsi requested review from dtzSiFive, Dinistro, mortbopet, lucas-rami, seldridge, teqdruid, fabianschuiki and maerhart as code owners July 25, 2023 20:48

Merge remote-tracking branch 'origin/main' into dev/darthscsi/modtmp1

aa9d490

teqdruid reviewed Jul 25, 2023

View reviewed changes

darthscsi added 2 commits July 25, 2023 15:59

missed one

224e891

missed one

0a58435

teqdruid approved these changes Jul 25, 2023

View reviewed changes

darthscsi added 4 commits July 25, 2023 17:07

fixups

38bb462

formatting

aa863d0

formatting

6260688

make clang-tidy happy

09084d6

darthscsi merged commit bf7d93f into main Jul 25, 2023
3 of 5 checks passed

darthscsi deleted the dev/darthscsi/modtmp1 branch April 11, 2024 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NFC] change ModulePortInfo to store in a single array and get rid of redundant APIs. This is preparation for not splitting inputs and outputs. #5680

[NFC] change ModulePortInfo to store in a single array and get rid of redundant APIs. This is preparation for not splitting inputs and outputs. #5680

darthscsi commented Jul 25, 2023

teqdruid Jul 25, 2023

darthscsi Jul 25, 2023

teqdruid Jul 25, 2023

teqdruid Jul 25, 2023

teqdruid Jul 25, 2023

teqdruid Jul 25, 2023

darthscsi Jul 25, 2023

teqdruid left a comment

teqdruid Jul 25, 2023

darthscsi Jul 25, 2023

teqdruid Jul 25, 2023

teqdruid Jul 25, 2023

darthscsi Jul 25, 2023

teqdruid Jul 25, 2023

darthscsi Jul 25, 2023

teqdruid Jul 25, 2023

teqdruid Jul 25, 2023

teqdruid Jul 25, 2023

teqdruid Jul 25, 2023

darthscsi Jul 25, 2023

teqdruid Jul 25, 2023

darthscsi Jul 25, 2023

teqdruid Jul 25, 2023

teqdruid Jul 25, 2023

darthscsi Jul 25, 2023

teqdruid Jul 25, 2023

[NFC] change ModulePortInfo to store in a single array and get rid of redundant APIs. This is preparation for not splitting inputs and outputs. #5680

[NFC] change ModulePortInfo to store in a single array and get rid of redundant APIs. This is preparation for not splitting inputs and outputs. #5680

Conversation

darthscsi commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teqdruid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment