Constant propagation opts #11790
Constant propagation opts #11790
Conversation
|
Added fix for #11785 @alexcrichton (missed that if const is 0, ctz, should be the type width), and tried to add runtests for srem. |
|
will run
|
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| ;; x % 1 == 0 | ||
| (rule (simplify_skeleton (urem x (iconst_u ty 1))) (iconst_u ty 0)) | ||
| (rule (simplify_skeleton (srem x (iconst_u ty 1))) (iconst_u ty 0)) | ||
| (rule (simplify_skeleton (srem x (iconst_s ty -1))) (iconst_u ty 0)) |
There was a problem hiding this comment.
How come this was removed? Was this an incorrect optimization?
There was a problem hiding this comment.
Yes - in combination with the urem const optimization led to optimizing imin rem -1 to 0 instead of having it trap which it should
There was a problem hiding this comment.
Ah that's actually a point where Rust and WebAssembly differ. Rust panicks on i32::MIN % -1 while WebAssembly returns 0. CLIF's semantics generally follow wasms, so I believe that this is still a correct optimization.
There was a problem hiding this comment.
Is that sufficient for CLIF (I guess at the least could cause issues in the rust cranelift backend if this occurs)?
There was a problem hiding this comment.
I'm not really sure what you mean by your question?
Regardless this isn't something we can really change. Wasm i32.rem_s is directly translated to a CLIF srem instruction. The CLIF instruction needs to mach the wasm instruction as a result, where the semantics of wasm is that i32::MIN % -1 is 0.
There was a problem hiding this comment.
@KGrewal1 I don't think we'll cause problems for cg_clif if we define srem to work with Wasm's semantics: we're defining it over a superset of the domain that Rust expects. Because other ISAs also work this way (e.g. aarch64 does not trap at all in divide/remainder operations), Rust already needs to reify its trapping semantics at a higher level; so the IR produced by cg_clif must necessarily contain the logic to panic on i32::MIN % -1, but that's already the case.
#11748 (comment)