HalfOpenRange/ClosedRange classes #4496

grg · 2024-03-01T23:39:48Z

New classes provide useful abstractions, such as le_bitrange/nw_bitrange.

Splitting out from #4489 at @fruffy's request.

@fruffy I've introduced a new helper struct RangeJsonHelpers that contains a pair std::function instances for the rangeToJSON/rangeFromJSON functions. The placeholder implementation of these functions call BUG with a message that the functionality is unimplemented. There's a function ir/bitrange.h:registerRangeJsonHelpers() that replaces the placeholders with functions that invoke the appropriate JSONGenerator/JSONLoader functions.

~~@ChrisDodd / @vlstill / @fruffy Do any of you have a better suggestion for how to handle JSON serialization/de-serialization. Here's the backstory/summary:~~

~~I'm porting the bitrange classes from Tofino to use le_bitrange in the midend def-use pass (Midend def-use pass #4489).~~
~~The open source compiler doesn't use le_bitrange and other HalfOpenRange/ClosedRange derivatives in the IR, so @fruffy suggested we put the code in lib/bitrange.h + cpp.~~
~~Tofino does use these classes in backend IR, and so requires the ability to serialize/deserialize the objects.~~
~~lib builds before the IR generator runs, and so lib code can't invoke JSONGenerator/JSONLoader functions since their header files indirectly use ir-generated.h.~~
I've introduced a helper struct with function instances for serialization/deserialization. The default functions are placeholders that call BUG. I've added a registerRangeJsonHelpers() in ir that registers real functions that correctly invoke JSONGenerator/JSONLoader if serialization/deserialization is required.

Refactored to avoid this ugliness after @vlstil's suggestion.

vlstill · 2024-03-04T13:15:34Z

@grg I think what you could do is:

Have a non-templated, freestanding rangeToJson/rangeFromJson just declared in bitrange.h.
Use these to implement the member functions in the bitranges.
Define these functions in some .cpp in the ir/ component.
I believe the linker should not complain for the lib component itself, and not even for something that uses it as the declared-only function is not used (and not virtual). The fromJson/toJson members are instantiated only when called anyway (as the bitrange is a template).
The linker would pull out the implementation from the ir normally unless someone links just lib and not ir and uses the fromJson/toJson (which is basically impossible as the loader/generator is also defined in ir).

Alternatively, you could even remove the member functions completely from bitrange and replace them by freestanding functions declared and defined in ir completely but that would probably be uglier.

grg · 2024-03-04T21:05:53Z

@grg I think what you could do is:

Have a non-templated, freestanding rangeToJson/rangeFromJson just declared in bitrange.h.

Use these to implement the member functions in the bitranges.

Define these functions in some .cpp in the ir/ component.

I believe the linker should not complain for the lib component itself, and not even for something that uses it as the declared-only function is not used (and not virtual). The fromJson/toJson members are instantiated only when called anyway (as the bitrange is a template).

The linker would pull out the implementation from the ir normally unless someone links just lib and not ir and uses the fromJson/toJson (which is basically impossible as the loader/generator is also defined in ir).

Alternatively, you could even remove the member functions completely from bitrange and replace them by freestanding functions declared and defined in ir completely but that would probably be uglier.

Thanks @vlstill. I should have tried this from the beginning. I incorrectly assumed that the linker would complain because it couldn't find implementations for rangeToJSON/rangeFromJSON. But you're correct that it only complains if the code calls those functions. (I tested this before updating the PR.)

fruffy

Very nice, thank you! Any chance there are tests we get for "free", i.e., they are already downstream? If not, that's fine.

lib/bitrange.h

asl · 2024-03-05T17:43:48Z

lib/bitrange.h

+ * creation of ranges in a more readable manner; use it to construct a
+ * HalfOpenRange or ClosedRange.
+ */
+struct FromTo {


Can these be put into some namespace? So the usage from the source code would be e.g. bitrange::FromTo(6, 8)?

Moved to BitRange namespace.

lib/bitrange.h

asl · 2024-03-05T17:46:14Z

@grg Lots of types are exposed to a default namespace here. It seems some are either implementation details and could be hidden or generic enough to be moved to a separate header. Some use of additional namespaces might provide some usage benefits IMO :)

grg · 2024-03-05T18:20:24Z

Very nice, thank you! Any chance there are tests we get for "free", i.e., they are already downstream? If not, that's fine.

I've pulled in the Tofino bitrange tests. I had to make a few small changes, but mostly came for free.

grg · 2024-03-05T19:19:05Z

@grg Lots of types are exposed to a default namespace here. It seems some are either implementation details and could be hidden or generic enough to be moved to a separate header. Some use of additional namespaces might provide some usage benefits IMO :)

The work I'm creating for myself while porting the Tofino midend def-use pass 🤣
I'll look at addressing some of these this afternoon.

fruffy · 2024-03-05T20:26:30Z

The work I'm creating for myself while porting the Tofino midend def-use pass 🤣

Well, your contributions are very much appreciated! 😄

grg · 2024-03-05T22:35:45Z

@asl Please take a look at the namespace updates.

ChrisDodd · 2024-03-06T20:44:41Z

lib/bitrange.h

+    const int quotient = dividend / divisor;
+    const int remainder = dividend % divisor;
+    if ((remainder != 0) && ((remainder < 0) != (divisor < 0))) return quotient - 1;
+    return quotient;


Does this get properly folded to a shift when divisor is a constant power of 2? Similarly, moduloFloor should get folded to an and

Testing in the Compiler Explorer:

clang: folds it to a right shift

gcc: does not fold to a right-shift
(I tested several different -O levels with gcc, but didn't get it to fold.)

Any suggestions how to restructure the code to get gcc to fold it to a shift?

And neither compiler does the fold to and for moduloFloor 🙁

Updated Compiler Explorer with both divideFloor and moduloFloor: https://godbolt.org/z/8a1KeYGon

I've got a solution to this, but I'm concerned about its portability. Will open a separate PR for that to get feedback after this has merged.

asl · 2024-03-06T21:34:37Z

lib/bitrange.h

+        return hi < other.hi;
+    }
+
+    friend size_t hash_value(const HalfOpenRange &r) {


Sorry, I missed this previously... While we are here, can we add std::hash / Util::Hasher specializations? Also, Utils::hash_combine combines std::hash<T>'s by default with T being int here, so I think it would be better to use return Utils::Hash{}(r.lo, r.hi) here, it would mix better (not sure it matters, but just in case).

Same for ClosedRange below.

I think I've implemented what you've requested with the specializations. Please take a look and let me know if anything is missing.

asl · 2024-03-06T21:35:17Z

@asl Please take a look at the namespace updates.

Thanks! This looks good to me, I'm just having some suggestion for hash usage. Sorry, I missed it before :(

asl

LGTM, Thanks!

New classes provide useful abstractions, such as le_bitrange/nw_bitrange. --------- Co-authored-by: Calin Cascaval <[email protected]> Co-authored-by: Chris Dodd <[email protected]> Co-authored-by: cole-barefoot <[email protected]> Co-authored-by: Jed Liu <[email protected]> Co-authored-by: Michal Kekely <[email protected]> Co-authored-by: Seth Fowler <[email protected]> Co-authored-by: Vojtěch Havel <[email protected]> Co-authored-by: Zhao Ma <[email protected]>

grg force-pushed the grgibb/bitrange_update branch 2 times, most recently from 62670aa to 7022b79 Compare March 2, 2024 02:24

grg marked this pull request as ready for review March 2, 2024 03:34

grg mentioned this pull request Mar 2, 2024

Midend def-use pass #4489

Merged

grg force-pushed the grgibb/bitrange_update branch from 7022b79 to f1f9c9e Compare March 4, 2024 21:03

grg requested review from vlstill, ChrisDodd and fruffy March 4, 2024 21:04

fruffy reviewed Mar 5, 2024

View reviewed changes

fruffy approved these changes Mar 5, 2024

View reviewed changes

fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Mar 5, 2024

asl reviewed Mar 5, 2024

View reviewed changes

lib/bitrange.h Show resolved Hide resolved

asl reviewed Mar 5, 2024

View reviewed changes

lib/bitrange.h Show resolved Hide resolved

grg force-pushed the grgibb/bitrange_update branch from 2b039e4 to dd65080 Compare March 5, 2024 22:39

ChrisDodd approved these changes Mar 6, 2024

View reviewed changes

asl reviewed Mar 6, 2024

View reviewed changes

asl approved these changes Mar 6, 2024

View reviewed changes

grg force-pushed the grgibb/bitrange_update branch from 813052e to 967fa6a Compare March 7, 2024 20:27

grg enabled auto-merge March 7, 2024 20:36

grg added this pull request to the merge queue Mar 7, 2024

Merged via the queue into main with commit da7807d Mar 7, 2024
17 checks passed

grg deleted the grgibb/bitrange_update branch March 7, 2024 22:03

grg mentioned this pull request Mar 7, 2024

help compiler folding for divideFloor/moduloFloor #4512

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HalfOpenRange/ClosedRange classes #4496

HalfOpenRange/ClosedRange classes #4496

grg commented Mar 1, 2024 •

edited

Loading

vlstill commented Mar 4, 2024

grg commented Mar 4, 2024 •

edited

Loading

fruffy left a comment

asl Mar 5, 2024

grg Mar 5, 2024

asl commented Mar 5, 2024

grg commented Mar 5, 2024

grg commented Mar 5, 2024

fruffy commented Mar 5, 2024

grg commented Mar 5, 2024

ChrisDodd Mar 6, 2024

grg Mar 6, 2024

grg Mar 6, 2024

grg Mar 6, 2024

grg Mar 7, 2024

asl Mar 6, 2024

grg Mar 6, 2024

asl commented Mar 6, 2024

asl left a comment

HalfOpenRange/ClosedRange classes #4496

HalfOpenRange/ClosedRange classes #4496

Conversation

grg commented Mar 1, 2024 • edited Loading

vlstill commented Mar 4, 2024

grg commented Mar 4, 2024 • edited Loading

fruffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asl commented Mar 5, 2024

grg commented Mar 5, 2024

grg commented Mar 5, 2024

fruffy commented Mar 5, 2024

grg commented Mar 5, 2024

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

asl commented Mar 6, 2024

asl left a comment

Choose a reason for hiding this comment

grg commented Mar 1, 2024 •

edited

Loading

grg commented Mar 4, 2024 •

edited

Loading