Revert "tuner: transform x coordinates to log2 scale."

This reverts commit 09c2351. The commit
changed the OFI NCCL plugin's region-based NCCL tuner to select
nonoptimal NCCL algorithm and protocol combinations for 0x7 AllGather
and 0x7 ReduceScatter.

Signed-off-by: Aviv Benchorin <benchori@amazon.com>

Author: avivbenchorin

include/tuner/nccl_ofi_tuner_region.h

@@ -5,7 +5,6 @@
 #ifndef NCCL_OFI_TUNER_REGION_H_
 #define NCCL_OFI_TUNER_REGION_H_
 
-#include <cmath>
 #include <stddef.h>
 #include "tuner/nccl_ofi_tuner_common.h"
 
@@ -58,28 +57,6 @@ typedef struct nccl_ofi_tuner_point
 {
 	double x;
 	double y;
-	enum COORD_SCALE {
-		UNSPECIFIED,
-		ORIGINAL,
-		X_LOG2
-
-	} coord_scale = UNSPECIFIED;
-
-	inline void transform_log2_x() {
-		if (coord_scale == X_LOG2) return;
-
-		if (x > 0) {
-			x = std::log2(x);
-			coord_scale = X_LOG2;
-		}
-	}
-
-	inline void transform_pow2_x() {
-		if (coord_scale != X_LOG2) return;
-
-		x = std::pow(2.0, x);
-		coord_scale = ORIGINAL;
-	}
 } nccl_ofi_tuner_point_t;
 
 typedef struct nccl_ofi_tuner_region {
@@ -93,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;
 
@@ -233,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_x();
-		}
-	}
-
 	return ncclSuccess;
 }
 
@@ -253,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_x();
-	b.transform_log2_x();
-	z.transform_log2_x();
-	ret.coord_scale = nccl_ofi_tuner_point_t::X_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_x();
+		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_x();
+		ret = (nccl_ofi_tuner_point_t){.x = z.x, .y = a.y};
 		return ret;
 	}
 
@@ -277,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_x();
 	return ret;
 }
 
@@ -1449,7 +1433,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_x();
 
 	/* Check all regions */
 	for (size_t i = 0; i < region_ctx->num_regions[collType] && in_out < 0; i++) {
@@ -1517,7 +1500,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_x();
 
 	/* 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 = (72.0 - 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 = 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){8.01, 1024},
+                                   (nccl_ofi_tuner_point_t){16, 1024},
                                    (nccl_ofi_tuner_point_t){TUNER_MAX_SIZE, TUNER_MAX_RANKS});
-    slope = (1024.0 - 64.0) / (log2(8.01) - log2(8.0));
-    projected_x = pow(2, ((TUNER_MAX_RANKS - 1024.0) / slope) + log2(8.01));
-    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_x();
-    p2_38M_16.transform_log2_x();
-    p3_4M_2.transform_log2_x();
-    p5_maxM_2.transform_log2_x();
-    e_48M_16_288M_128.transform_log2_x();
-    printf("INFO extended point after transform_log2_x: %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,
@@ -116,7 +98,6 @@ static int test_is_inside_region(void) {
                      p3_4M_2,
                      p5_maxM_2}};
 
-
     /* Points on the vertices of the polygon should be classified to be on the edge of the region */
     if (is_inside_region(e_48M_16_288M_128, &region) != 0)
         return -1;
@@ -135,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::X_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));
@@ -154,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},
@@ -173,30 +152,25 @@ 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 - 0.1, e_48M_16_288M_128.y - 10.0, nccl_ofi_tuner_point_t::X_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_x();
-        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, 6},
+    const nccl_ofi_tuner_point_t outside_vertices[] = {{8.0 * 1024 * 1024, 4},
                                                        {8.0 * 1024 * 1024, 32},
                                                        {8.0 * 1024 * 1024, 128},
                                                        {8.0 * 1024 * 1024, 512},
                                                        {8.0 * 1024 * 1024, TUNER_MAX_RANKS},
-                                                       {16.0 * 1024 * 1024, 10},
+                                                       {16.0 * 1024 * 1024, 8},
                                                        {16.0 * 1024 * 1024, 32},
                                                        {16.0 * 1024 * 1024, 128},
                                                        {16.0 * 1024 * 1024, 512},
@@ -206,7 +180,7 @@ static int test_is_inside_region(void) {
                                                        {32.0 * 1024 * 1024, 64},
                                                        {32.0 * 1024 * 1024, 128},
                                                        {32.0 * 1024 * 1024, 256},
-                                                       {64 * 1024 * 1024, 35},
+                                                       {64 * 1024 * 1024, 32},
                                                        {64.0 * 1024 * 1024, 64},
                                                        {64.0 * 1024 * 1024, 256},
                                                        {64.0 * 1024 * 1024, 1024},
@@ -217,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;
         };
     }