[analysis] Implement a vector lattice #6058
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
| } | ||
| result = GREATER; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
That we copy-paste this switch logic is kind of disappointing. I'd have hoped with all the template magic there would be a way to avoid duplication? The same for the functions below.
There was a problem hiding this comment.
You mean the copy-paste between the array and vector lattices? I could probably try to use even more template magic to reduce the duplication, but I think at that point we could start calling it template obfuscation 😅
There was a problem hiding this comment.
Can they both inherit from something?
Or perhaps they can both simply call a standalone utility class that does these operations?
There was a problem hiding this comment.
We could perhaps have a CRTP parent class, but I don't think it's worth it. These methods are simple enough that I think there's more value in being able to read their implementations directly without an extra layer of source indirection than there would be in deduplicating them.
There was a problem hiding this comment.
What would be the harm in adding a utility function (not a CRTP parent class) "compareSingleLatticeElement" that contains the switch, and is called from those loops? Those functions would become much smaller and more readable, and we'd avoid the duplication.
There was a problem hiding this comment.
The types are different, so at least that utility would have to be another template. It would also have to go in some new header file but would not be part of any public API, which is unfortunate. I don't anticipate the utility would be used anywhere except these two places, either.
I can do it if you feel strongly about it, but I don't think the extra complexity is worth the deduplication.
There was a problem hiding this comment.
I don't feel that strongly about it, I suppose.
| elem.push_back(l->lattice.makeElement()); | ||
| } | ||
| return ElementImpl{std::move(elem)}; | ||
| } | ||
| WASM_UNREACHABLE("unexpected lattice"); |
There was a problem hiding this comment.
I won't ask for a change here, but I really do think virtual would be a nicer way to write code like this, rather than have to keep modifying RandomFullLattice for each type. That is, I would personally gone the other route and had a virtual method that this class calls, each new lattice just implements a "makeRandom" interface, so it all plugs in.
There was a problem hiding this comment.
I plan to at least get rid of the distinction between RandomFullLattice and RandomLattice, which should simplify things.
I can also clean up this giant if-else chain by using std::visit combined with overloaded free functions, which will be closer to what you're envisioning, I think. I don't think that going all the way to creating an inheritance-based wrapper for each of the lattices would be worth it, though.
tlively
force-pushed
the
array-lattice
branch
from
f62c7aa to
8601941
Compare
tlively
force-pushed
the
vector-lattice
branch
from
c5a6478 to
8efecb1
Compare
The vector lattice is nearly identical to the array lattice, except that the size of the elements is specified at runtime when the lattice object is created rather than at compile time. The code and tests are largely copy-pasted and fixed up from the array implementation, but there are a couple differences. First, initializing vector elements does not need the template magic used to initialize array elements. Second, the obvious implementations of join and meet do not work for vectors of bools because they might be specialized to be bit vectors, so we need workarounds for that particular case.
tlively
force-pushed
the
vector-lattice
branch
from
8efecb1 to
b6b33b5
Compare
The vector lattice is nearly identical to the array lattice, except that the size of the elements is specified at runtime when the lattice object is created rather than at compile time. The code and tests are largely copy-pasted and fixed up from the array implementation, but there are a couple differences. First, initializing vector elements does not need the template magic used to initialize array elements. Second, the obvious implementations of join and meet do not work for vectors of bools because they might be specialized to be bit vectors, so we need workarounds for that particular case.
The vector lattice is nearly identical to the array lattice, except that the
size of the elements is specified at runtime when the lattice object is created
rather than at compile time. The code and tests are largely copy-pasted and
fixed up from the array implementation, but there are a couple differences.
First, initializing vector elements does not need the template magic used to
initialize array elements. Second, the obvious implementations of join and meet
do not work for vectors of bools because they might be specialized to be bit
vectors, so we need workarounds for that particular case.