Fix souper harvest#13225
Conversation
Co-authored-by: Copilot <copilot@github.com>
| /// However, this introduces several type mismatches in souper IR. | ||
| /// For example, a CLIF instruction usually has operands of at least 8 bits, | ||
| /// while Souper/LLVM allows `i1` operands. Therefore, when a comparison and | ||
| /// a bitwise operation are combined together, we end up with a type mismatch in | ||
| /// souper IR. | ||
| /// | ||
| /// ```clif | ||
| /// v3 = icmp eq v1, v2 | ||
| /// v4 = bor v3, v3 | ||
| /// ``` | ||
| /// | ||
| /// ```souper | ||
| /// %3:i1 = eq %1, %2 | ||
| /// %4:i8 = or %3, %3 | ||
| /// ``` |
There was a problem hiding this comment.
Would it be good enough to eagerly translate CLIF icmp instructions into two Souper instructions: a compare followed by a zero extend? Eg something like this:
;; CLIF
v3 = icmp eq v1, v2
v4 = bor v3, v3
;; Souper
%3:i1 = eq %1, %2
%4:i8 = zext %3
%5:i8 = or %4, %4I think this would be a bit simpler and lets us avoid the SouperValue machinery, while also fixing the same set of type mismatch bugs? But I'm not 100% sure on that, so I'm interested in your feedback and whether you considered this approach.
There was a problem hiding this comment.
Oh, haven't considered this approach, and I agree this would make this PR much simpler...
In addition, having the bitwidth semantics aligned (or i8 vs or i1) is a huge plus!
Imagine that or is being used by other instructions, then the i1 type of the or would affect the user instructions. And this effect would be descended further on and on. Restoring the type of the comparison immediately with zext will rule out this scenario.
I will come back once I complete implementing your approach. Thanks for the insight!
|
@fitzgen Change the implementation.
One thing to keep in mind: |
Currently,
clif-util souper-harvesthas limitations where:bnotinstruction to Souper IR, andicmpinstruction.The fix for the first one is straightforward. I added a translation from
bnot x(in CLIF) toxor x, -1(in Souper IR).The second problem arises when an instruction uses a value produced by an
icmpinstruction.In Cranelift,
icmpproduces ani8value; therefore, its users usually also are ofi8type too.However, in Souper/LLVM,
icmpproduces ani1value; therefore, its users, without casting, users must be ofi1type.The current problem of the harvest is that, when determining the type of an Souper value, it directly copies the type of that of CLIF side. For example,
is translated to
which causes type mismatch, preventing Souper from running properly. (Notice that
orisi8!)I fixed this by carrying Souper-side type information with new
SouperValuestruct, while such a type information is produced insouper_type_offunction. Initial type informations are only produced for constants (iconst), variables, and type-casts.However, I didn't yet implement type unification. A proper implementation would require a larger refactor to make operand types mutable and propagate inferred types back to other values. The current change is intentionally narrower to keep this simple.
Test Results