Sam Parker (May 04 2022 at 10:45): Sam Parker (May 04 2022 at 10:45):Hi @Chris Fallin
I just pulled down your if-let changes and auto rebased my local branch (which was building fine), but I'm getting some unexpected issues in generated_code.rs relating to patterns that, I'm pretty sure, I'm not touching. The errors all look like:
error[E0317]: if may be missing an else clause
...
| ^^ expected enum core::option::Option, found ()
|
= note: expected enum core::option::Option<reg::Reg>
found unit type ()
Have you see this before?
Chris Fallin (May 04 2022 at 16:28):@Sam Parker I will take a look! I have not seen this before
Chris Fallin (May 04 2022 at 16:36):@Sam Parker on current main (527b7a9b0594c6d9cc4b5c93db81871f356793a5) I don't see any issue: I tried regenerating ISLE to be sure (cargo check -p cranelift-codegen --features rebuild-isle,all-arch) and all generated source typechecks, with no diffs vs what's checked in
Chris Fallin (May 04 2022 at 16:36):do you have local changes triggering this? I'm happy to help debug if you can give me a diff
Chris Fallin (May 04 2022 at 16:38):(if you see this in your tomorrow (Thu) then I'm out on Thu/Fri but will take a look as soon as I can)
Sam Parker (May 05 2022 at 07:44):Yes, sorry, it has to be my changes (my main is fine) but I just don't understand, yet, how my new simd patterns are affecting some existing scalar patterns and wondered if you'd ever seen something similar. After looking all at the problem patterns, they all shared the use of iconst and I have been playing with constants :)
Sam Parker (May 05 2022 at 10:10):I've removed most of my changes, yet I still haven't figured out the root cause... But from my naive perspective it looks like the generated constructors are now being written in the wrong order; an example of generated code diff look like this:
pub fn constructor_put_nonzero_in_reg_sext64<C: Context>(ctx: &mut C, arg0: Value) -> Option<Reg> {
let pattern0_0 = arg0;
+ // Rule at src/isa/aarch64/lower.isle line 444.
+ let expr0_0 = constructor_put_in_reg_sext64(ctx, pattern0_0)?;
+ let expr1_0 = constructor_trap_if_zero_divisor(ctx, expr0_0)?;
+ return Some(expr1_0);
let pattern1_0 = C::value_type(ctx, pattern0_0);
if let Some(pattern2_0) = C::def_inst(ctx, pattern0_0) {
let pattern3_0 = C::inst_data(ctx, pattern2_0);
if let &InstructionData::UnaryImm {
opcode: ref pattern4_0,
imm: pattern4_1,
} = &pattern3_0
{
if let &Opcode::Iconst = pattern4_0 {
if let Some(pattern6_0) = C::nonzero_u64_from_imm64(ctx, pattern4_1) {
// Rule at src/isa/aarch64/lower.isle line 449.
let expr0_0 = constructor_imm(ctx, pattern1_0, pattern6_0)?;
return Some(expr0_0);
}
}
}
}
- // Rule at src/isa/aarch64/lower.isle line 444.
- let expr0_0 = constructor_put_in_reg_sext64(ctx, pattern0_0)?;
- let expr1_0 = constructor_trap_if_zero_divisor(ctx, expr0_0)?;
- return Some(expr1_0);
}
And my changes to lower.isle are minimal:
;; Extract the low half components of rn.
;; tmp1 = |c|a|
- (tmp1 Reg (xtn64 rn $false))
+ (tmp1 Reg (xtn64 rn))
;; Sum the respective high half components.
;; rd = |dg+ch|be+af||dg+ch|be+af|
@@ -273,7 +273,7 @@
;; Extract the low half components of rm.
;; tmp2 = |g|e|
- (tmp2 Reg (xtn64 rm $false))
+ (tmp2 Reg (xtn64 rm))
I've now tried three completely separate isle touching local branches, and they're all affected in the same way. It's actually possible to reproduce this by just reordering the bconst lower rules, so I'm assuming any change at the source level can reproduce this bug.
fitzgen (he/him) (May 05 2022 at 15:22):(fyi: Chris is out for the rest of the week; he will be back on Monday)
fitzgen (he/him) (May 05 2022 at 15:23):I suspect this has to do with the recent changes to ISLE's trie IR where priority ranges were removed and replaced with scalar priorities. Probably just a bug that fell out during that change, and not fundamental to the range -> scalar change.
fitzgen (he/him) (May 05 2022 at 15:25):probably the same thing as: https://github.com/bytecodealliance/wasmtime/pull/4093#issuecomment-1118654081
Chris Fallin (May 05 2022 at 16:13):(hi from the airport) yes, I think reverting that PR is the right move and I'll figure it out on Monday. Sorry for the trouble!
fitzgen (he/him) (May 05 2022 at 16:57):https://github.com/bytecodealliance/wasmtime/pull/4102
Sam Parker (May 10 2022 at 13:22):Thanks, I can confirm that reordering the bconst rules doesn't result in illegal generated_code. Something is still going wrong for me though. On one branch I can still see my original issue and on another, which should only touch the aarch64 backend, I'm seeing changes for x64 and s390x along with test failures for s390x... So, I can only assume that I'm building things incorrectly. Any ideas?
Sam Parker (May 10 2022 at 13:53):Okay, fixed the odd x64 and s390x issues by manually reverting the changes, not sure how those changes ended up in the commit but also confused as to why they aren't regenerated (correctly)
Chris Fallin (May 10 2022 at 15:52):Ah, you're probably hitting the same issue I did: the manifest indicated things were up to date, so the compiler fix didn't cause a rebuild for you locally
Chris Fallin (May 10 2022 at 15:53):This is yet more evidence to me that we need to do something better IMHO; I'll think a bit more and follow up on the "don't check in source" issue
Sam Parker (May 10 2022 at 17:57):Yes, I agree. I think I've spent as much time figuring out build quirks as writing the patches and that isn't right, plus I'm yet to solve all my problems :)
Last updated: Dec 23 2024 at 13:07 UTC