mpl2: fix macro blockage penalty

[AcKoucher]

This PR is to:

1. Change the way we compute the penalty for overlapping between clusters with macros and macro blockages during SA for SoftMacros. We now just compute the overlapping area between the SoftMacro and the macro_blockage.
2. Adapt the calculation of cluster placement penalties (SoftSA) to the fixed sequence pair.
3. Ensure we actually use the new sequence pair in when running enhanced macro placement function.

For documentation: Not only were we using an overly complex expression to compute the penalty, but there was also a bug: even when there was no overlap, the computed macro_blockage_penalty between a SoftMacro and a macro_blockage was different from 0, because, even if the ratio of one of the coordinates was 0 the other one was not. This was due to way std::max was being used in the ratio of each coordinate.

To keep the original computing, we would have to ensure that, if there was no overlap between a SoftMacro and amacro_blockage in any of the coordinates, the penalty would be 0.

[ktbarrett]

I would think it should be a cast to GPI_GENARRAY.

I agree. This is a generate loop. I think an array of instances to be exactly that. An array of interfaces is very clearly an array of hierarchy objects and would map there too.

I can't map vpiInstanceArray / vpiInterfaceArray to GPI_GENARRAY because it needs vpi_iterate with vpiRange.

So the VPI assumes that everything that's a GPI_GENARRAY is a pseudo handle instead of an actual handle like vpiInstanceArray or vpiInterfaceArray. Is that's the issue? That's a really bad assumption IMO. I think we need a new object type that encodes pseudo-handle logic and a separate one that handles actual hierarchical array logic. Both map to GPI_GENARRAY, but with different implementations.

I'll have to look into the Questa and Riviera results when I get home.

[AndrewNolte]

I still don't really understand what a pseudo-region is, I think gpi.h needs a lot more clarification. My perception is it's the same as hierarchy arrays or hierarchy scopes, and sometimes the handle may be missing? However in the vpiimpl nothing in to_gpi_objtype returns GPI_GEN_ARRAY, so we only have code that treats these handles as if they're not there. Or if they are there, for convenience uses the GPI_ARRAY to avoid iterating, instead doing size/index/range discovery

[ktbarrett]

I think gpi.h needs a lot more clarification

You are absolutely correct there. I'm not certain myself how a pseudo-region works. But it is an implementation detail, not necessarily an API concern. GPI_GENARRAY should have well defined semantics regardless if the impl is a pseudo-region or an actual handle. Pseudo-regions as I understand are a hack because some simulators don't support vpiGenArrayScope or similar. The pseudo-handle is there is group the actual objects it can find, which is each of the loop's vpiGenScopes.

The entire C++ codebase needs a major facelift, but it's not public API, so it's lower on my own radar than Python rn. Sweeping C++ changes are a 3.0 issue. Until then we just need something that works, even if it's ugly as sin.

[marlonjames]

See the wiki for some details on pseudo-regions.

[marlonjames]

For VPI, there are actual differences between an instance array and gen scope array.

We need a way to distinguish hierarchy arrays like vpiInterfaceArray from things like vpiRegArray, rather than wade into the mess of GPI_GENARRAY and pseudo-regions.

[ktbarrett]

Is the difference in terms of implementation or capability? Still seems like it's an array of scopes, which is kind of what GPI_GENARRAY is.

[marlonjames]

Conceptually GPI_GENARRAY is as you say, an array of scopes. But GPI code uses it only for pseudo-regions.

In fact, vpiGenScopeArray handles are thrown away in favor of pseudo-region objects when looking up gen scope array by name:

https://github.com/cocotb/cocotb/blob/fdf7286a3a38b703dc4a3bb7c707bfcd2263773a/src/cocotb/share/lib/vpi/VpiImpl.cpp#L367-L380

We don't iterate over vpiGenScopeArray, so during iteration only vpiGenScope shows up.

When using VPI, gen scope and gen scope array objects are not instance objects like modules, packages, and interfaces are, and need to be handled differently internally.

So I think for instance arrays, we either need to differentiate GPI_ARRAY between hierarchy (instance arrays) and non-hierarchy (var array, net array), or add another GPI_* type.

Pseudo-region implementation needs an update, and maybe at that time we could merge GPI_GENARRAY into whatever type we use for other hierarchy arrays.

[ktbarrett]

Rerunning CI.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 72.64%. Comparing base (ed01008) to head (2b6c3a5). Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3704      +/-   ##
==========================================
- Coverage   72.67%   72.64%   -0.03%     
==========================================
  Files          49       49              
  Lines        8050     8050              
  Branches     2212     2218       +6     
==========================================
- Hits         5850     5848       -2     
- Misses       1689     1695       +6     
+ Partials      511      507       -4     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[ktbarrett]

Since #3824 landed first, you'll need to update the test expectations. Use positive logic, not not verilator.

[ktbarrett]

Riviera failures.

# COUT:   3000.00ns INFO     cocotb.regression                  running test_sv_if.test_sv_intf_arr_access (4/5)
# COUT:                                                             Test that interface array objects can be accessed
# COUT:   3000.00ns ERROR    gpi                                Invalid Index - Index 1 is not in the range of [0:0]
# COUT:   3000.00ns WARNING  gpi                                Failed to find a handle at index 1 via any registered implementation
# COUT:   4000.00ns INFO     cocotb.regression                  test_sv_if.test_sv_intf_arr_access failed
# COUT:                                                         Traceback (most recent call last):
# COUT:                                                           File "/opt/actions-runner/_work/cocotb/cocotb/tests/test_cases/test_sv_interface/test_sv_if.py", line 43, in test_sv_intf_arr_access
# COUT:                                                             assert hasattr(dut.sv_if_arr[i], "a")
# COUT:                                                           File "/opt/actions-runner/_work/cocotb/cocotb/.nox/dev_test_sim-sim-riviera-toplevel_lang-verilog-gpi_interface-vpi/lib/python3.8/site-packages/cocotb/handle.py", line 772, in __getitem__
# COUT:                                                             raise IndexError(f"{self._path} contains no object at index {index}")
# COUT:                                                         IndexError: top.sv_if_arr contains no object at index 1
# COUT:   4000.00ns INFO     cocotb.regression                  running test_sv_if.test_sv_intf_arr_iteration (5/5)
# COUT:                                                             Test that interface arrays can be iterated
# COUT:   5000.00ns INFO     cocotb.regression                  test_sv_if.test_sv_intf_arr_iteration failed
# COUT:                                                         Traceback (most recent call last):
# COUT:                                                           File "/opt/actions-runner/_work/cocotb/cocotb/tests/test_cases/test_sv_interface/test_sv_if.py", line 58, in test_sv_intf_arr_iteration
# COUT:                                                             assert count == 3
# COUT:                                                         AssertionError: assert 1 == 3