Revert "tuner: transform coordinates to log-log scale"

This reverts commit 75b1002.
Reason: I should first merge aws#1082 (tuner region test CI),
 and then resubmit this change. That way, any region tuner decision change can be caught easily.

E.g. such diff is found by running the tuner region test CI manually on this commmit (to be reverted here):
One diff that caught my eyes:
in tests/integration/test-output/tuner_ranks8_nodes256.out
This line is missing:
256   2048  34359738368   ReduceScatter  Ring  Simple        0.0       P6
Which means on P6, with 256 nodes, the it will not switch eventually to
Ring Simple. This is something I need to investigate more.

Author: fengjica

include/tuner/nccl_ofi_tuner_region.h

@@ -5,8 +5,6 @@
 #ifndef NCCL_OFI_TUNER_REGION_H_
 #define NCCL_OFI_TUNER_REGION_H_
 
-#include <cassert>
-#include <cmath>
 #include <stddef.h>
 #include "tuner/nccl_ofi_tuner_common.h"
 
@@ -59,47 +57,6 @@ typedef struct nccl_ofi_tuner_point
 {
 	double x;
 	double y;
-	enum COORD_SCALE {
-		UNSPECIFIED,
-		ORIGINAL,
-		LOG2
-
-	} coord_scale = UNSPECIFIED;
-
-	inline void transform_log2() {
-		if (coord_scale == LOG2) {
-			assert(false && "Coordinate already in LOG2 scale");
-			return;
-		}
-
-		if (x >= 0 && y >= 0) {
-			// for 0, set to a small positive number for log2().
-			const double eps = 1e-6;
-			if (x == 0) {
-				x = eps;
-			}
-			if (y == 0) {
-				y = eps;
-			}
-
-			x = std::log2(x);
-			y = std::log2(y);
-			coord_scale = LOG2;
-		} else {
-			assert(false && "Invalid coordinates for LOG2 transformation");
-		}
-	}
-
-	inline void transform_pow2() {
-		if (coord_scale != LOG2) {
-			assert(false && "Coordinate not in LOG2 scale for POW2 transformation");
-			return;
-		}
-
-		x = std::pow(2.0, x);
-		y = std::pow(2.0, y);
-		coord_scale = ORIGINAL;
-	}
 } nccl_ofi_tuner_point_t;
 
 typedef struct nccl_ofi_tuner_region {
@@ -113,8 +70,7 @@ nccl_ofi_tuner_point_t extend_region(nccl_ofi_tuner_point_t a,
 									 nccl_ofi_tuner_point_t b,
 									 nccl_ofi_tuner_point_t z);
 
-int is_inside_region(
-	nccl_ofi_tuner_point_t point,
-	const nccl_ofi_tuner_region_t *region);
+int is_inside_region(nccl_ofi_tuner_point_t point,
+					 nccl_ofi_tuner_region_t *region);
 
 #endif /* NCCL_OFI_TUNER_REGION_H_ */

src/tuner/nccl_ofi_regions.cpp

@@ -127,7 +127,7 @@ static inline double distance(nccl_ofi_tuner_point_t x,
 			      double eps)
 {
 	nccl_ofi_tuner_point_t dy = vsub(y1, y0);
-	nccl_ofi_tuner_point_t x1, s = {0, 0};
+	nccl_ofi_tuner_point_t x1, s;
 	int r;
 
 	x1.x = x.x + dy.y;
@@ -155,12 +155,12 @@ static inline double distance(nccl_ofi_tuner_point_t x,
  * 		-1 for outside
  * 		0 for on edge.
  */
-int is_inside_region(nccl_ofi_tuner_point_t point, const nccl_ofi_tuner_region_t *region)
+int is_inside_region(nccl_ofi_tuner_point_t point, nccl_ofi_tuner_region_t *region)
 {
 	assert(region->num_vertices > 1);
 
 	size_t i, k;
-	const nccl_ofi_tuner_point_t *pv;
+	nccl_ofi_tuner_point_t *pv;
 	double min_x, max_x, min_y, max_y;
 	const double eps = 1e-10;
 
@@ -201,7 +201,6 @@ int is_inside_region(nccl_ofi_tuner_point_t point, const nccl_ofi_tuner_region_t
 
 	/* Pick a point far enough to be outside any polygon */
 	nccl_ofi_tuner_point_t e = {.x = 2.0 * TUNER_MAX_SIZE, .y = 2.0 * TUNER_MAX_RANKS};
-	e.transform_log2();
 
 	int crosses = 0;
 	int intersectResult = -1;
@@ -234,14 +233,6 @@ static ncclResult_t set_regions(nccl_ofi_tuner_region_context_t *region_ctx,
 	}
 
 	memcpy(region_ctx->regions[collType], &regions[0], num_regions * sizeof(nccl_ofi_tuner_region_t));
-
-	for (size_t i = 0; i < num_regions; i++) {
-		nccl_ofi_tuner_region& region = region_ctx->regions[collType][i];
-		for (size_t j = 0; j < region.num_vertices; j++) {
-			region.vertices[j].transform_log2();
-		}
-	}
-
 	return ncclSuccess;
 }
 
@@ -254,22 +245,15 @@ nccl_ofi_tuner_point_t extend_region(nccl_ofi_tuner_point_t a, nccl_ofi_tuner_po
 {
 	nccl_ofi_tuner_point_t ret;
 
-	a.transform_log2();
-	b.transform_log2();
-	z.transform_log2();
-	ret.coord_scale = nccl_ofi_tuner_point_t::LOG2;
-
 	if (a.x == b.x) {
 		/* a and b are on the same vertical line */
-		ret.x = a.x, ret.y = z.y;
-		ret.transform_pow2();
+		ret = (nccl_ofi_tuner_point_t){.x = a.x, .y = z.y};
 		return ret;
 	}
 
 	if (a.y == b.y) {
 		/* a and b are on the same horizontal line */
-		ret.x = z.x, ret.y = a.y;
-		ret.transform_pow2();
+		ret = (nccl_ofi_tuner_point_t){.x = z.x, .y = a.y};
 		return ret;
 	}
 
@@ -278,12 +262,11 @@ nccl_ofi_tuner_point_t extend_region(nccl_ofi_tuner_point_t a, nccl_ofi_tuner_po
 	double projected_zy = m * z.x + c;
 
 	if (projected_zy < z.y) {
-		ret.x = z.x, ret.y = projected_zy;
+		ret = (nccl_ofi_tuner_point_t){.x = z.x, .y = projected_zy};
 	} else {
-		ret.x = (z.y - c) / m, ret.y = z.y;
+		ret = (nccl_ofi_tuner_point_t){.x = (z.y - c) / m, .y = z.y};
 	}
 
-	ret.transform_pow2();
 	return ret;
 }
 
@@ -1999,7 +1982,6 @@ ncclResult_t region_get_coll_info_internal_v2(nccl_ofi_tuner_context_t *ctx,
 
 	p.x = (double)nBytes;
 	p.y = (double)region_ctx->dims.num_ranks;
-	p.transform_log2();
 
 	/* Check all regions */
 	for (size_t i = 0; i < region_ctx->num_regions[collType] && in_out < 0; i++) {
@@ -2067,7 +2049,6 @@ ncclResult_t region_get_coll_info_internal_v3(nccl_ofi_tuner_context_t *ctx,
 
 	p.x = (double)nBytes;
 	p.y = (double)region_ctx->dims.num_ranks;
-	p.transform_log2();
 
 	/* Check all regions */
 	for (size_t i = 0; i < region_ctx->num_regions[collType] && in_out < 0; i++) {

tests/unit/region_based_tuner.cpp

@@ -11,11 +11,6 @@
 #include "tuner/nccl_ofi_tuner_region.h"
 #include "nccl_ofi_param.h"
 
-using std::abs;
-using std::log2;
-using std::pow;
-const double eps = 1e-4;
-
 static int test_extend_region(void)
 {
     nccl_ofi_tuner_point_t extended_point;
@@ -26,9 +21,8 @@ static int test_extend_region(void)
     extended_point = extend_region((nccl_ofi_tuner_point_t){2, 8},
                                    (nccl_ofi_tuner_point_t){4, 8},
                                    (nccl_ofi_tuner_point_t){TUNER_MAX_SIZE, TUNER_MAX_RANKS});
-    if (abs(extended_point.x - TUNER_MAX_SIZE) > eps || extended_point.y != 8) {
-        printf("X-Axis Extend Test Failed : Extended Points : x = %f (diff = %f) y = %f\n", extended_point.x,
-            extended_point.x - TUNER_MAX_SIZE, extended_point.y);
+    if (extended_point.x != TUNER_MAX_SIZE || extended_point.y != 8) {
+        printf("X-Axis Extend Test Failed : Extended Points : x = %f y = %f\n", extended_point.x, extended_point.y);
         return -1;
     }
 
@@ -45,26 +39,22 @@ static int test_extend_region(void)
     extended_point = extend_region((nccl_ofi_tuner_point_t){8, 64},
                                    (nccl_ofi_tuner_point_t){8290304, 72},
                                    (nccl_ofi_tuner_point_t){TUNER_MAX_SIZE, TUNER_MAX_RANKS});
-    slope = (log2(72.0) - log2(64.0)) / (log2(8290304.0) - log2(8.0)); // slope = (y2 - y1)/(x2 - x1)
+    slope = (72.0 - 64.0) / (8290304.0 - 8.0); // slope = (y2 - y1)/(x2 - x1)
     // y3 = mx3 + c and substitute for m=(y2-y1)/(x2-x1) and c = y2 - mx2
-    projected_y = pow(2.0, log2(72.0) + slope * (log2(TUNER_MAX_SIZE) - log2(8290304.0))); // y3 = y2 + mx3 - mx2
-    if (abs(extended_point.x - TUNER_MAX_SIZE) > eps || extended_point.y != projected_y) {
-        printf("X-Axis Upper Bound Test Failed : Extended Points : x = %f (diff = %f) y = %f (diff = %f) \n",
-            extended_point.x, extended_point.x - TUNER_MAX_SIZE,
-            extended_point.y, extended_point.y - projected_y);
+    projected_y = 72.0 + slope * (TUNER_MAX_SIZE - 8290304.0); // y3 = y2 + mx3 - mx2
+    if (extended_point.x != TUNER_MAX_SIZE || extended_point.y != projected_y) {
+        printf("X-Axis Upper Bound Test Failed : Extended Points : x = %f y = %f\n", extended_point.x, extended_point.y);
         return -1;
     }
 
     /* Extend the line to TUNER_MAX_RANKS (y-axis) */
     extended_point = extend_region((nccl_ofi_tuner_point_t){8, 64},
                                    (nccl_ofi_tuner_point_t){16, 1024},
                                    (nccl_ofi_tuner_point_t){TUNER_MAX_SIZE, TUNER_MAX_RANKS});
-    slope = (log2(1024.0) - log2(64.0)) / (log2(16) - log2(8.0));
-    projected_x = pow(2, ((log2(TUNER_MAX_RANKS) - log2(1024.0)) / slope) + log2(16));
-    if (abs(extended_point.x - projected_x) > eps || extended_point.y != TUNER_MAX_RANKS) {
-        printf("X-Axis Upper Bound Test 2 Failed : Extended Points : x = %f (diff = %f) y = %f (diff = %f) \n",
-            extended_point.x, extended_point.x - projected_x,
-            extended_point.y, extended_point.y - TUNER_MAX_RANKS);
+    slope = (1024.0 - 64.0) / (16.0 - 8.0);
+    projected_x = ((TUNER_MAX_RANKS - 1024.0) / slope) + 16;
+    if (extended_point.x != projected_x || extended_point.y != TUNER_MAX_RANKS) {
+        printf("X-Axis Upper Bound Test Failed : Extended Points : x = %f y = %f\n", extended_point.x, extended_point.y);
         return -1;
     }
 
@@ -85,7 +75,7 @@ static int test_extend_region(void)
 |                 .                                      |
 |            .                                           |
 |--------*------*----------*----------*---------*--------*---
-|    p3(4M, 2)                                           |p5(TUNER_MAX_SIZE, 2))
+|    p3(4M, 2)                                           |p4(TUNER_MAX_SIZE, 2))
 |                                                        |
 */
 static int test_is_inside_region(void) {
@@ -97,14 +87,6 @@ static int test_is_inside_region(void) {
         (nccl_ofi_tuner_point_t){(double)48.0 * 1024 * 1024, 16},
         (nccl_ofi_tuner_point_t){(double)288.0 * 1024 * 1024, 128},
         (nccl_ofi_tuner_point_t){TUNER_MAX_SIZE, TUNER_MAX_RANKS});
-    printf("INFO extended point: %f %f \n", e_48M_16_288M_128.x, e_48M_16_288M_128.y );
-
-    p1_288M_128.transform_log2();
-    p2_38M_16.transform_log2();
-    p3_4M_2.transform_log2();
-    p5_maxM_2.transform_log2();
-    e_48M_16_288M_128.transform_log2();
-    printf("INFO extended point after transform_log2: %f %f \n", e_48M_16_288M_128.x, e_48M_16_288M_128.y );
 
     nccl_ofi_tuner_region_t region = {
         .algorithm = NCCL_ALGO_RING,
@@ -134,17 +116,15 @@ static int test_is_inside_region(void) {
     To find the points on the edge of the polygons:
     1. Consider two vertices of the polygon
     2. Calculate the slope and y-intercept of the line.
-    3. Using the equation y = m * x + c, get multiple points on the line in powers of 2.
+    3. Using the equation y = mx + c, get multiple points on the line in powers of 2.
     */
     for (size_t i = 0; i < region.num_vertices; i++) {
         size_t k = (i + 1) % region.num_vertices;
         double slope = (region.vertices[k].y - region.vertices[i].y) / (region.vertices[k].x - region.vertices[i].x);
         double c = region.vertices[k].y - (slope * (region.vertices[i].x));
         for (double x = region.vertices[i].x; x < region.vertices[k].x; x = x * 2) {
             double y = (slope * x) + c;
-            nccl_ofi_tuner_point_t test_point {x, y, nccl_ofi_tuner_point_t::LOG2};
-
-            if (is_inside_region(test_point, &region) != 0)
+            if (is_inside_region((nccl_ofi_tuner_point_t){x, y}, &region) != 0)
                 return -1;
             // printf(" Is (%.10f, %.10f) inside the region : %d\n", x, y, is_inside_region(
             //     (nccl_ofi_tuner_point_t){x, y}, &region));
@@ -153,8 +133,8 @@ static int test_is_inside_region(void) {
 
     printf("All points on the edges of the polygon are detected correcltly\n");
 
-    const size_t num_points = 20;
-    nccl_ofi_tuner_point_t inside_vertices[] = {{16.0 * 1024 * 1024, 4},
+    size_t num_points = 20;
+    const nccl_ofi_tuner_point_t inside_vertices[] = {{16.0 * 1024 * 1024, 4},
                                                       {128.0 * 1024 * 1024, 4},
                                                       {1.0 * 1024 * 1024 * 1024, 4},
                                                       {4.0 * 1024 * 1024 * 1024, 4},
@@ -172,24 +152,19 @@ static int test_is_inside_region(void) {
                                                       {32.0 * 1024 * 1024 * 1024, 128},
                                                       {64.0 * 1024 * 1024 * 1024, 128},
                                                       {64.0 * 1024 * 1024 * 1024, 256},
-                                                      // Note, set a big enough diff (10.0) below, otherwise
-                                                      // the delta after log2 is within floating error (eps).
-                                                      {TUNER_MAX_SIZE - 10.0, 128},
-                                                      {e_48M_16_288M_128.x - 1.0, e_48M_16_288M_128.y - 10.0, nccl_ofi_tuner_point_t::LOG2}};
+                                                      {TUNER_MAX_SIZE - 1.0, 128},
+                                                      {e_48M_16_288M_128.x - 1.0, e_48M_16_288M_128.y - 1.0}};
 
     /* These points should be inside the polygon */
     for (size_t i = 0; i < num_points; i++) {
-        inside_vertices[i].transform_log2();
-        int d = is_inside_region(inside_vertices[i], &region);
-        if (d != 1) {
-            printf("%ld: %.10f, %.10f is_inside_region: %d\n", i, inside_vertices[i].x, inside_vertices[i].y, d);
+        if (is_inside_region(inside_vertices[i], &region) != 1) {
+            printf("%.10f, %.10f\n", inside_vertices[i].x, inside_vertices[i].y);
             return -1;
         };
     }
 
     printf("All points inside the polygon are detected correcltly\n");
 
-    const size_t outside_num_points = 24;
     const nccl_ofi_tuner_point_t outside_vertices[] = {{8.0 * 1024 * 1024, 4},
                                                        {8.0 * 1024 * 1024, 32},
                                                        {8.0 * 1024 * 1024, 128},
@@ -216,10 +191,9 @@ static int test_is_inside_region(void) {
                                                        {e_48M_16_288M_128.x + 1.0, e_48M_16_288M_128.y + 1.0}};
 
     /* These points should be outside the polygons */
-    for (size_t i = 0; i < outside_num_points; i++) {
-        int d = is_inside_region(outside_vertices[i], &region);
-        if ( d != -1) {
-            printf("%ld: %.10f, %.10f is_inside_region: %d\n", i, outside_vertices[i].x, outside_vertices[i].y, d);
+    for (size_t i = 0; i < num_points; i++) {
+        if (is_inside_region(outside_vertices[i], &region) != -1) {
+            printf("%.10f, %.10f\n", outside_vertices[i].x, outside_vertices[i].y);
             return -1;
         };
     }
@@ -240,17 +214,13 @@ static int test_is_inside_region(void) {
 }
 
 int main(int argc, const char **argv) {
-    int ret = 0;
-    if ((ret |= test_extend_region()) < 0) {
+    if (test_extend_region() < 0) {
         printf("Extend Region test failed\n");
     };
 
-    if ((ret |= test_is_inside_region()) < 0) {
+    if (test_is_inside_region() < 0) {
         printf("Is inside region function failed\n");
     }
 
-    if (ret == 0) {
-        printf("All tests passed.\n");
-    }
-    return ret;
+    return 0;
 }