Subzero. Removes references to %ah.

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

[John, 2015-07-28 15:22:24]

jpp@chromium.org changed reviewers:
+ jfb@chromium.org, jvoung@chromium.org, kschimpf@google.com,
stichnot@chromium.org

[Jim Stichnoth, 2015-07-28 15:58:23]

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#new...
src/IceRegistersX8632.h:91: assert(Reg_GPR_First <= RegNum && RegNum <=
Reg_ebx);
I must have missed this before, but now that it's in my sights...

For asserts like
  assert(A && B);
I prefer breaking it into
  assert(A);
  assert(B);
which gives more precise information when the assertion fails.

https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1853: Variable *T_eax = makeReg(IceType_i16,
Traits::RegisterSet::Reg_eax);
Add a comment explaining the ah workaround.

https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1924: _set_dest_nonkillable();
I don't think any of these new instances of _set_dest_nonkillable() are needed,
can you double-check?

That method is poorly named, sorry about that.  Generally you only need to call
it after inserting an instruction where the dest variable appears in a source
operand of the instruction except for Dest==Srcs[0].  The Dest==Srcs[0] case is
handled as part of postLower().  For any other source operands, or if Srcs[0] is
e.g. a mem operand that contains Dest (such as in some lea cases), calling it is
needed.

https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1933: _shr(T_eax, Ctx->getConstantInt8(8));
Consider using X86_CHAR_BIT instead of 8.

[jvoung (off chromium), 2015-07-28 16:17:45]

https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1853: Variable *T_eax = makeReg(IceType_i16,
Traits::RegisterSet::Reg_eax);
For T_eax, why not use IceType_i32 instead of IceType_i16, like in the Urem
case?  Should be smaller without the 0x66 prefix?

[John, 2015-07-28 18:21:17]

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#new...
src/IceRegistersX8632.h:91: assert(Reg_GPR_First <= RegNum && RegNum <=
Reg_ebx);
On 2015/07/28 15:58:23, stichnot wrote:
> I must have missed this before, but now that it's in my sights...
> 
> For asserts like
>   assert(A && B);
> I prefer breaking it into
>   assert(A);
>   assert(B);
> which gives more precise information when the assertion fails.

Nothing that a simple

:%s/assert(\([^&]\+\) && \([^)]\+\))/assert(\1);assert(\2)

won't solve. :)

https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1853: Variable *T_eax = makeReg(IceType_i16,
Traits::RegisterSet::Reg_eax);
On 2015/07/28 16:17:45, jvoung wrote:
> For T_eax, why not use IceType_i32 instead of IceType_i16, like in the Urem
> case?  Should be smaller without the 0x66 prefix?

Regardless of being smaler, using i32 is more efficient and what I intended.
Thanks for catching this.

https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1853: Variable *T_eax = makeReg(IceType_i16,
Traits::RegisterSet::Reg_eax);
On 2015/07/28 15:58:23, stichnot wrote:
> Add a comment explaining the ah workaround.

Done.

https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1924: _set_dest_nonkillable();
On 2015/07/28 15:58:23, stichnot wrote:
> I don't think any of these new instances of _set_dest_nonkillable() are
needed,
> can you double-check?
> 
> That method is poorly named, sorry about that.  Generally you only need to
call
> it after inserting an instruction where the dest variable appears in a source
> operand of the instruction except for Dest==Srcs[0].  The Dest==Srcs[0] case
is
> handled as part of postLower().  For any other source operands, or if Srcs[0]
is
> e.g. a mem operand that contains Dest (such as in some lea cases), calling it
is
> needed.

Done.

https://codereview.chromium.org/1260163003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1933: _shr(T_eax, Ctx->getConstantInt8(8));
On 2015/07/28 15:58:23, stichnot wrote:
> Consider using X86_CHAR_BIT instead of 8.

The name X86_CHAR_BIT is somewhat confusing. In C CHAR_BIT represents the number
of bits in a char (which you can't portably trust to always be 8.) I would
rather leave an 8 (or use a named constant, e.g., AlSizeInBytes.) In the future
I would like to rename X86_CHAR_BITS to X86_BYTE_SIZE or something else.

[Jim Stichnoth, 2015-07-28 21:20:10]

lgtm

[John, 2015-07-28 23:56:34]

Committed patchset #3 (id:40001) manually as
448c16f0f6905460a3d27e728ed0f14a1c08ff69 (presubmit successful).

[jvoung (off chromium), 2015-07-29 22:23:53]

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

[native-client-reviews_googlegroups.com, 2015-07-30 18:55:21]

Thanks for the report. I already have a fix for it.

On Wed, Jul 29, 2015 at 3:23 PM, <jvoung@chromium.org> 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
>
>
>
> 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.

[Jim Stichnoth, 2015-07-31 20:13:10]

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.

[native-client-reviews_googlegroups.com, 2015-07-31 20:42:20]

+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.