Bug 201703 – Elide unnecessary moves in Air O0

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Elide unnecessary moves in Air O0

[Keith Miller]

Created attachment 378593 [details]
Patch

[Keith Miller]

Created attachment 378594 [details]
Patch

[Keith Miller]

Created attachment 378607 [details]
Patch

[Keith Miller]

Created attachment 378609 [details]
Patch

[Saam Barati]

Comment on attachment 378609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378609&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        Elide unnecessary moves in Air O0

can you explain how?

> Source/JavaScriptCore/ChangeLog:10
> +        temp dies as it could be reused again later. Thus every temp,

no comma

> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:451
> +                if (source.isAddr() || dest.isAddr())
> +                    return false;

why not just check that both are tmps? Seems easier to follow. Does your code not break for constants if you don't do this, since you unconditionally access reg() below

> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:456
> +                Reg reg = source.isTmp() ? m_map[source.tmp()].reg : source.reg();

this seems wrong. you access source.reg() if it's not a tmp. However, you can't be a reg if you're not a tmp

> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:458
> +                if (!reg)
> +                    return false;

probably not super important, but we could just say dest is now spilled at this location instead of returning false

> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:478
> +            if (elideMove())
> +                goto elided;

This is wrong. For example, you now get clobberedRegisters incorrectly now

[Keith Miller]

Comment on attachment 378609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378609&action=review

>> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:451
>> +                    return false;
> 
> why not just check that both are tmps? Seems easier to follow. Does your code not break for constants if you don't do this, since you unconditionally access reg() below

Yeah, I was thinking Regs were not temps. I think this code is cleaner in that world... unfortunately that's not reality though. I'll fix this.

>> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:456
>> +                Reg reg = source.isTmp() ? m_map[source.tmp()].reg : source.reg();
> 
> this seems wrong. you access source.reg() if it's not a tmp. However, you can't be a reg if you're not a tmp

See above. Will fix.

>> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:458
>> +                    return false;
> 
> probably not super important, but we could just say dest is now spilled at this location instead of returning false

I was thinking about that but it seems like we might want to materialize the value into a register though if register pressure is now low.

>> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:478
>> +                goto elided;
> 
> This is wrong. For example, you now get clobberedRegisters incorrectly now

Hmm, yeah, maybe I should just mark move needsToGenerate up here.

[Keith Miller]

Created attachment 378695 [details]
Patch

[Keith Miller]

Created attachment 378697 [details]
Patch

[Keith Miller]

Created attachment 378701 [details]
Patch

[Saam Barati]

Comment on attachment 378701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378701&action=review

> Source/JavaScriptCore/ChangeLog:14
> +        This appears to be a minor progression on the overall score of
> +        wasm subtests in JS2 and a 10% wasm-JIT memory usage reduction.

these results still hold?

> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:438
> +                Reg reg = source.isReg() ? source.reg() : m_map[source.tmp()].reg;

nit: let's call this sourceReg

Also, this looks wrong. What if the value supposed to be in $rax is actually spilled? Don't you always want to consult m_map? An instruction may ask for a named register, but it doesn't mean the corresponding value is already in that register.

If you read allocNamed below, you'll see the problem. We might load, say, $rax, from where it is on the stack back into $rax. That is ignored here.

> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:-619
> -                bool needsToGenerate = true;

can you assert it's false here? Personally, I'd just keep this variable, since they're kinda unrelated

[Keith Miller]

Comment on attachment 378701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378701&action=review

>> Source/JavaScriptCore/ChangeLog:14
>> +        wasm subtests in JS2 and a 10% wasm-JIT memory usage reduction.
> 
> these results still hold?

Yeah, seems like it.

>> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:438
>> +                Reg reg = source.isReg() ? source.reg() : m_map[source.tmp()].reg;
> 
> nit: let's call this sourceReg
> 
> Also, this looks wrong. What if the value supposed to be in $rax is actually spilled? Don't you always want to consult m_map? An instruction may ask for a named register, but it doesn't mean the corresponding value is already in that register.
> 
> If you read allocNamed below, you'll see the problem. We might load, say, $rax, from where it is on the stack back into $rax. That is ignored here.

I see. Yeah, that's a good point. I think it might be worthwhile to skip named source registers for now. Let try retesting without them.

>> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:-619
>> -                bool needsToGenerate = true;
> 
> can you assert it's false here? Personally, I'd just keep this variable, since they're kinda unrelated

Yeah, I could imagine it changing over time though. I'll add the assert.

[Keith Miller]

Created attachment 378932 [details]
Patch

[Keith Miller]

Created attachment 378936 [details]
Patch

[Saam Barati]

Comment on attachment 378936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378936&action=review

I think you still have a bug

> Source/JavaScriptCore/ChangeLog:9
> +        This patch also removes the code that would try to reuse
> +        temps. That code makes it hard to accurately determine where a

"reuse temps" => "reuse temps in WasmAirIRGenerator"

> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:435
> +                // FIXME: We don't track where the last use of a reg is globally so we don't know where we can't elide them.

"can't elide" => "can elide"

> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:448
> +                if (dest.isReg())
> +                    return dest.reg() != sourceReg;

don't you need to update the mapping here still? Seems wrong to not do that.

> Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:51
> +                usedCalleeSaves.merge(inst.extraEarlyClobberedRegs());

👍🏼

[Keith Miller]

Created attachment 379008 [details]
Patch

[Keith Miller]

Created attachment 379011 [details]
Patch

[Saam Barati]

Comment on attachment 379011 [details]
Patch

r=me

Let me see if I can get a test to work

[Keith Miller]

Committed r250009: <https://trac.webkit.org/changeset/250009>

[Radar WebKit Bug Importer]

<rdar://problem/55462115>