Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(103)

Issue 1260163003: Subzero. Removes references to %ah. (Closed)

Created:
5 years, 4 months ago by John
Modified:
5 years, 4 months ago
CC:
native-client-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Removes references to ah. AH is a thorn in the flesh for our X86-64 backend. The assembler was designed to always encode the low 8-bit registers, so %ah would become %spl. While it is true we **could** force %spl to always be encoded as %ah, that would not work if the instruction has a rex prefix. This CL removes references to %ah from TargetX86Base. There used to be 2 uses of ah in the target lowering: 1) To zero-extend %al before an unsigned div: mov <<src0>>, %al mov 0, %ah div <<src1>> This pattern has been changed to xor %eax, %eax mov <<src0>>, %al div <<src1>> 2) To access the 8-bit remainder for 8-bit division: mov %ah, <<dest>> This pattern has been changed to shr $8, %eax mov %al, <<Dest>> BUG= https://code.google.com/p/nativeclient/issues/detail?id=4077 R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=448c16f0f6905460a3d27e728ed0f14a1c08ff69

Patch Set 1 #

Total comments: 10

Patch Set 2 : Addresses comments. #

Patch Set 3 : git pull #

Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -34 lines) Patch
M src/IceInstX8632.def View 2 chunks +1 line, -8 lines 0 comments Download
M src/IceInstX8664.def View 1 2 1 chunk +0 lines, -5 lines 0 comments Download
M src/IceRegistersX8632.h View 1 2 chunks +8 lines, -6 lines 0 comments Download
M src/IceRegistersX8664.h View 1 2 1 chunk +6 lines, -2 lines 0 comments Download
M src/IceTargetLoweringX86BaseImpl.h View 1 3 chunks +40 lines, -13 lines 0 comments Download

Messages

Total messages: 11 (1 generated)
John
5 years, 4 months ago (2015-07-28 15:22:25 UTC) #2
Jim Stichnoth
Thanks! https://codereview.chromium.org/1260163003/diff/1/src/IceRegistersX8632.h File src/IceRegistersX8632.h (right): https://codereview.chromium.org/1260163003/diff/1/src/IceRegistersX8632.h#newcode91 src/IceRegistersX8632.h:91: assert(Reg_GPR_First <= RegNum && RegNum <= Reg_ebx); I ...
5 years, 4 months ago (2015-07-28 15:58:23 UTC) #3
jvoung (off chromium)
https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86BaseImpl.h File src/IceTargetLoweringX86BaseImpl.h (right): https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86BaseImpl.h#newcode1853 src/IceTargetLoweringX86BaseImpl.h:1853: Variable *T_eax = makeReg(IceType_i16, Traits::RegisterSet::Reg_eax); For T_eax, why not ...
5 years, 4 months ago (2015-07-28 16:17:45 UTC) #4
John
https://codereview.chromium.org/1260163003/diff/1/src/IceRegistersX8632.h File src/IceRegistersX8632.h (right): https://codereview.chromium.org/1260163003/diff/1/src/IceRegistersX8632.h#newcode91 src/IceRegistersX8632.h:91: assert(Reg_GPR_First <= RegNum && RegNum <= Reg_ebx); On 2015/07/28 ...
5 years, 4 months ago (2015-07-28 18:21:17 UTC) #5
Jim Stichnoth
lgtm
5 years, 4 months ago (2015-07-28 21:20:10 UTC) #6
John
Committed patchset #3 (id:40001) manually as 448c16f0f6905460a3d27e728ed0f14a1c08ff69 (presubmit successful).
5 years, 4 months ago (2015-07-28 23:56:34 UTC) #7
jvoung (off chromium)
On 2015/07/28 23:56:34, John wrote: > Committed patchset #3 (id:40001) manually as > 448c16f0f6905460a3d27e728ed0f14a1c08ff69 (presubmit ...
5 years, 4 months ago (2015-07-29 22:23:53 UTC) #8
native-client-reviews_googlegroups.com
Thanks for the report. I already have a fix for it. On Wed, Jul 29, ...
5 years, 4 months ago (2015-07-30 18:55:21 UTC) #9
Jim Stichnoth
On 2015/07/29 22:23:53, jvoung wrote: > On 2015/07/28 23:56:34, John wrote: > > Committed patchset ...
5 years, 4 months ago (2015-07-31 20:13:10 UTC) #10
native-client-reviews_googlegroups.com
5 years, 4 months ago (2015-07-31 20:42:20 UTC) #11
Message was sent while issue was closed.
+1 for "not suppress the consistency check in the entry node," regardless
of how.

I would suggest is to add FakeDefs for (all?) arguments.

On Fri, Jul 31, 2015 at 1:13 PM, <stichnot@chromium.org> wrote:

> On 2015/07/29 22:23:53, jvoung wrote:
>
>> On 2015/07/28 23:56:34, John wrote:
>> > Committed patchset #3 (id:40001) manually as
>> > 448c16f0f6905460a3d27e728ed0f14a1c08ff69 (presubmit successful).
>>
>
> Hmm, this change might be hitting a new liveness assert:
>>
>
> [225]    //%__LLVM ERROR: Fatal inconsistency in liveness analysis
>>
>
> LiveOrig-Live = %__220:al %__234:al %__248:al %__262:al %__276:al %__290:al
>> %__304:al %__318:al %__332:al %__346:al %__360:al %__374:al %__388:al
>>
> %__402:al
>
>> %__416:al
>>
>
>
> Some of the verbose output around a urem:
>>
>
> [148]    //%__128 = add i32 %__30, 14
>> [149]    //%__129 = add i32 %__2, %__128
>> [XXX]    //%__130 = load i8, i8* %__129, align 1
>> [150]    //%__130 = load i8, i8* [%__2+%__128], align 1
>> [729]  mov.i32 %__410, %__30
>> [730]  %__410 = add.i32 %__410, 14
>> [731]  mov.i32 %__128, %__410
>> [243]    //%__131 = urem i8 [%__2+%__128], 62
>> [151]    //%__131 = urem i8 %__130, 62 // LIVEEND={%__130}
>> [732]  mov.i32 %__411, %__2
>> [733]  mov.i32 %__412, %__128
>> [734]  mov.i8 %__413, 62
>> [735]  %__414:eax = def.pseudo
>> [736]  %__414:eax = xor.i32 %__414:eax, %__414:eax
>> [737]  mov.i8 %__415:al, [%__411+%__412]
>> [738]  %__416:al = div.i8 %__416:al, %__413, %__415:al
>> [739]  %__414:eax = shr.i32 %__414:eax, 8
>> [740]  mov.i8 %__131, %__416:al
>> [741]  use.pseudo %__414:eax
>> [152]    //%__132 = zext i8 %__131 to i32 // LIVEEND={%__131}
>> [742]  %__417 = movz.i8 %__131
>> [743]  mov.i32 %__132, %__417
>> [153]    //%__133 = add i32 @Global860, %__132 // LIVEEND={%__132}
>> [744]  mov.i32 %__418, %__132
>> [745]  %__418 = add.i32 %__418, @Global860
>> [746]  mov.i32 %__133, %__418
>> [154]    //%__134 = load i8, i8* %__133, align 1 // LIVEEND={%__133}
>>
>
>
>
> toolchain_build/src/subzero/pnacl-sz -O2 --filetype=obj -o /tmp/temp.o
>>
>
>
>
pnacl/build/llvm-test-suite/MultiSource/Applications/sqlite3/Output/sqlite3.final.pexe
>
> I can send you the sqlite3.final.pexe, or you can build it via:
>>
>
> toolchain_build/toolchain_build_pnacl.py --testsuite-sync --sync-only
>>
>
> PNACL_CONCURRENCY=16 ./pnacl/scripts/llvm-test.py --testsuite-all
>>
> --arch=x86-32
>
>> --verbose --opt O2b_sz
>>
>
> I was wondering why this didn't trigger in our existing tests,
> specifically the
> div/rem tests in 8bit.pnacl.ll.
>
> It turns out to be because the check for "Fatal inconsistency in liveness
> analysis" is suppressed in the entry node.  Otherwise, you can have a
> function
> argument whose only use is in the entry node, in which case that argument
> appears to be a single-block variable that is live on entry to the block,
> triggering the validation failure.
>
> A "workaround" to trigger the urem problem in our lit tests, is to begin
> the
> functions like this:
>   entry:
>     br label %block1
>   block1:
>
> Maybe a better way is to unconditionally mark arguments as multi-block and
> then
> not suppress the consistency check in the entry node.  I will look into
> that.
>
> https://codereview.chromium.org/1260163003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

Powered by Google App Engine
This is Rietveld 408576698