Remove isValidBelForCell

This Arch API dates from when we were first working out how to
implement placement validity checking, and in practice is little used by
the core parts of placer1/HeAP and the Arch implementation involves a
lot of duplication with isBelLocationValid.

In the short term; placement validity checking is better served by the
combination of checkBelAvail and isValidBelForCellType before placement;
followed by isBelLocationValid after placement (potentially after
moving/swapping multiple cells).

Longer term, removing this API makes things a bit cleaner for a new
validity checking API.

Signed-off-by: gatecat <gatecat@ds0.me>

4 files changed, 38 insertions, 24 deletions

diff --git a/common/nextpnr.h b/common/nextpnr.h
index b6ee33fe..4ddf8fef 100644
--- a/common/nextpnr.h
+++ b/common/nextpnr.h
@@ -1169,7 +1169,6 @@ template <typename R> struct ArchAPI : BaseCtx
     virtual BelBucketId getBelBucketByName(IdString name) const = 0;
     virtual BelBucketId getBelBucketForBel(BelId bel) const = 0;
     virtual BelBucketId getBelBucketForCellType(IdString cell_type) const = 0;
-    virtual bool isValidBelForCell(CellInfo *cell, BelId bel) const = 0;
     virtual bool isBelLocationValid(BelId bel) const = 0;
     virtual typename R::CellTypeRangeT getCellTypes() const = 0;
     virtual typename R::BelBucketRangeT getBelBuckets() const = 0;
@@ -1420,7 +1419,6 @@ template <typename R> struct BaseArch : ArchAPI<R>
     {
         return getBelBucketByName(cell_type);
     };
-    virtual bool isValidBelForCell(CellInfo *cell, BelId bel) const override { return true; }
     virtual bool isBelLocationValid(BelId bel) const override { return true; }
     virtual typename R::CellTypeRangeT getCellTypes() const override
     {
diff --git a/common/place_common.cc b/common/place_common.cc
index 6526c38e..e5b48ffb 100644
--- a/common/place_common.cc
+++ b/common/place_common.cc
@@ -118,8 +118,7 @@ bool place_single_cell(Context *ctx, CellInfo *cell, bool require_legality)
         }
         IdString targetType = cell->type;
         for (auto bel : ctx->getBels()) {
-            if (ctx->isValidBelForCellType(targetType, bel) &&
-                (!require_legality || ctx->isValidBelForCell(cell, bel))) {
+            if (ctx->isValidBelForCellType(targetType, bel)) {
                 if (ctx->checkBelAvail(bel)) {
                     wirelen_t wirelen = get_cell_metric_at_bel(ctx, cell, bel, MetricType::COST);
                     if (iters >= 4)
@@ -155,12 +154,20 @@ bool place_single_cell(Context *ctx, CellInfo *cell, bool require_legality)
             ctx->unbindBel(ripup_target->bel);
             best_bel = ripup_bel;
         } else {
+            ripup_target = nullptr;
             all_placed = true;
         }
+        ctx->bindBel(best_bel, cell, STRENGTH_WEAK);
+        if (require_legality && !ctx->isBelLocationValid(best_bel)) {
+            ctx->unbindBel(best_bel);
+            if (ripup_target != nullptr) {
+                ctx->bindBel(best_bel, ripup_target, STRENGTH_WEAK);
+            }
+            all_placed = false;
+            continue;
+        }
         if (ctx->verbose)
             log_info("   placed single cell '%s' at '%s'\n", cell->name.c_str(ctx), ctx->nameOfBel(best_bel));
-        ctx->bindBel(best_bel, cell, STRENGTH_WEAK);
-
         cell = ripup_target;
     }
     return true;
@@ -387,7 +394,7 @@ class ConstraintLegaliseWorker
                         for (auto bel : ctx->getBelsByTile(cp.second.x, cp.second.y)) {
                             CellInfo *belCell = ctx->getBoundBelCell(bel);
                             if (belCell != nullptr && !solution.count(belCell->name)) {
-                                if (!ctx->isValidBelForCell(belCell, bel)) {
+                                if (!ctx->isBelLocationValid(bel)) {
                                     NPNR_ASSERT(belCell->belStrength < STRENGTH_STRONG);
                                     ctx->unbindBel(bel);
                                     rippedCells.insert(belCell->name);
diff --git a/common/placer1.cc b/common/placer1.cc
index 280dd02e..270430e9 100644
--- a/common/placer1.cc
+++ b/common/placer1.cc
@@ -167,13 +167,6 @@ class SAPlacer
                                   "\'%s\' of type \'%s\'\n",
                                   loc_name.c_str(), bel_type.c_str(ctx), cell->name.c_str(ctx), cell->type.c_str(ctx));
                     }
-                    if (!ctx->isValidBelForCell(cell, bel)) {
-                        IdString bel_type = ctx->getBelType(bel);
-                        log_error("Bel \'%s\' of type \'%s\' is not valid for cell "
-                                  "\'%s\' of type \'%s\'\n",
-                                  loc_name.c_str(), bel_type.c_str(ctx), cell->name.c_str(ctx), cell->type.c_str(ctx));
-                    }
-
                     auto bound_cell = ctx->getBoundBelCell(bel);
                     if (bound_cell) {
                         log_error(
@@ -182,6 +175,12 @@ class SAPlacer
                     }
 
                     ctx->bindBel(bel, cell, STRENGTH_USER);
+                    if (!ctx->isBelLocationValid(bel)) {
+                        IdString bel_type = ctx->getBelType(bel);
+                        log_error("Bel \'%s\' of type \'%s\' is not valid for cell "
+                                  "\'%s\' of type \'%s\'\n",
+                                  loc_name.c_str(), bel_type.c_str(ctx), cell->name.c_str(ctx), cell->type.c_str(ctx));
+                    }
                     locked_bels.insert(bel);
                     placed_cells++;
                 }
@@ -444,7 +443,7 @@ class SAPlacer
             IdString targetType = cell->type;
 
             auto proc_bel = [&](BelId bel) {
-                if (ctx->isValidBelForCellType(targetType, bel) && ctx->isValidBelForCell(cell, bel)) {
+                if (ctx->isValidBelForCellType(targetType, bel)) {
                     if (ctx->checkBelAvail(bel)) {
                         uint64_t score = ctx->rng64();
                         if (score <= best_score) {
@@ -480,10 +479,20 @@ class SAPlacer
                 ctx->unbindBel(ripup_target->bel);
                 best_bel = ripup_bel;
             } else {
+                ripup_target = nullptr;
                 all_placed = true;
             }
             ctx->bindBel(best_bel, cell, STRENGTH_WEAK);
 
+            if (!ctx->isBelLocationValid(best_bel)) {
+                ctx->unbindBel(best_bel);
+                if (ripup_target != nullptr) {
+                    ctx->bindBel(best_bel, ripup_target, STRENGTH_WEAK);
+                }
+                all_placed = false;
+                continue;
+            }
+
             // Back annotate location
             cell->attrs[ctx->id("BEL")] = ctx->getBelName(cell->bel).str(ctx);
             cell = ripup_target;
diff --git a/common/placer_heap.cc b/common/placer_heap.cc
index 7882c8da..7d529401 100644
--- a/common/placer_heap.cc
+++ b/common/placer_heap.cc
@@ -392,13 +392,6 @@ class HeAPPlacer
                               "\'%s\' of type \'%s\'\n",
                               loc_name.c_str(), bel_type.c_str(ctx), cell->name.c_str(ctx), cell->type.c_str(ctx));
                 }
-                if (!ctx->isValidBelForCell(cell, bel)) {
-                    IdString bel_type = ctx->getBelType(bel);
-                    log_error("Bel \'%s\' of type \'%s\' is not valid for cell "
-                              "\'%s\' of type \'%s\'\n",
-                              loc_name.c_str(), bel_type.c_str(ctx), cell->name.c_str(ctx), cell->type.c_str(ctx));
-                }
-
                 auto bound_cell = ctx->getBoundBelCell(bel);
                 if (bound_cell) {
                     log_error("Cell \'%s\' cannot be bound to bel \'%s\' since it is already bound to cell \'%s\'\n",
@@ -406,6 +399,12 @@ class HeAPPlacer
                 }
 
                 ctx->bindBel(bel, cell, STRENGTH_USER);
+                if (!ctx->isBelLocationValid(bel)) {
+                    IdString bel_type = ctx->getBelType(bel);
+                    log_error("Bel \'%s\' of type \'%s\' is not valid for cell "
+                              "\'%s\' of type \'%s\'\n",
+                              loc_name.c_str(), bel_type.c_str(ctx), cell->name.c_str(ctx), cell->type.c_str(ctx));
+                }
                 placed_cells++;
             }
         }
@@ -571,12 +570,13 @@ class HeAPPlacer
                         place_cells.push_back(ci);
                         placed = true;
                     } else {
-                        if (ctx->isValidBelForCell(ci, bel)) {
-                            ctx->bindBel(bel, ci, STRENGTH_STRONG);
+                        ctx->bindBel(bel, ci, STRENGTH_STRONG);
+                        if (ctx->isBelLocationValid(bel)) {
                             cell_locs[cell.first].locked = true;
                             placed = true;
                             bels_used.insert(bel);
                         } else {
+                            ctx->unbindBel(bel);
                             available_bels.at(ci->type).push_front(bel);
                         }
                     }