Bug 194656

Summary: lowerStackArgs should lower Lea32/64 on ARM64 to Add
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 194036    
Attachments:
Description Flags
WIP
none
WIP
none
patch ysuzuki: review+

Saam Barati
Reported 2019-02-14 09:51:25 PST
Because it's not really an address. It's just an add on arm. So we end up using the wrong computation since we think we have a scaled address.
Attachments
WIP (3.77 KB, patch)
2019-02-14 09:53 PST, Saam Barati 		no flags
DetailsFormatted DiffDiff
WIP (3.78 KB, patch)
2019-02-14 15:31 PST, Saam Barati 		no flags
DetailsFormatted DiffDiff
patch (5.09 KB, patch)
2019-02-14 17:41 PST, Saam Barati 		ysuzuki: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Saam Barati
Comment 1 2019-02-14 09:53:07 PST
Created attachment 362031 [details] WIP I want to add some more tests. But the lowering itself is done
Saam Barati
Comment 2 2019-02-14 15:31:07 PST
Created attachment 362070 [details] WIP
Saam Barati
Comment 3 2019-02-14 17:41:53 PST
Created attachment 362086 [details] patch
Yusuke Suzuki
Comment 4 2019-02-14 20:16:05 PST
Comment on attachment 362086 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=362086&action=review r=me, I understand the issue, interesting. > Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp:77 > + if (isARM64() && (inst.kind.opcode == Lea32 || inst.kind.opcode == Lea64)) { > + // On ARM64, Lea is just an add. We can't handle this below because > + // taking into account the Width to see if we can compute the immediate > + // is wrong. Should we add some notes in AirOpcode.opcodes? Basically, UA (UseAddr) typed address would cause a similar problem. (Currently, only LEA uses this type).
Saam Barati
Comment 5 2019-02-14 20:22:41 PST
Comment on attachment 362086 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=362086&action=review >> Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp:77 >> + // is wrong. > > Should we add some notes in AirOpcode.opcodes? Basically, UA (UseAddr) typed address would cause a similar problem. (Currently, only LEA uses this type). I don’t fully follow this comment. Can you expand on it. I feel like I’m missing something. What do you mean by UseAdder? What do you mean by “typed adresses”?
Saam Barati
Comment 6 2019-02-14 20:39:54 PST
Oh I see what you mean. I’ll add a comment
Saam Barati
Comment 7 2019-02-14 22:38:13 PST
landed in: https://trac.webkit.org/changeset/241577/webkit
Radar WebKit Bug Importer
Comment 8 2019-02-14 22:41:36 PST
<rdar://problem/48101457>
Note You need to log in before you can comment on or make changes to this bug.