Subzero: Removed unnecessary global access checks

Subzero: Removed unnecessary global access checks

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=181a9bcb55b8d1e22210479e5210852d703855f5

[tlively, 2016-07-27 01:23:26]

tlively@google.com changed reviewers:
+ kschimpf@google.com, mcgrathr@chromium.org, stichnot@chromium.org

[tlively, 2016-07-27 01:23:27]

Is there any way to perform a load or store at an offset off of a global without
introducing an intermediary variable to store the address? Subzero seems to
support such an offset but I'm not sure what the bitcode would look like. If
this is possible I need to write a test with it.

[Jim Stichnoth, 2016-07-27 13:19:34]

https://codereview.chromium.org/2183683003/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2183683003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:319: ConstantRelocatable *Reloc =
llvm::dyn_cast<ConstantRelocatable>(Op);
auto *Reloc

https://codereview.chromium.org/2183683003/diff/1/src/IceASanInstrumentation.h
File src/IceASanInstrumentation.h (right):

https://codereview.chromium.org/2183683003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.h:38: : Instrumentation(Ctx), GlobalSizes(), RzNum(0)
{
You probably don't need to explicitly specify the default initializer for
GlobalSizes.

[Jim Stichnoth, 2016-07-27 13:28:18]

On 2016/07/27 01:23:27, tlively wrote:
> Is there any way to perform a load or store at an offset off of a global
without
> introducing an intermediary variable to store the address? Subzero seems to
> support such an offset but I'm not sure what the bitcode would look like. If
> this is possible I need to write a test with it.

You can't specify it directly in bitcode, but Subzero's address mode inference
is what constructs the IR.  For example:

@string = internal global [7 x i8] c"abcdefg"
define internal i32 @test() {
  %base = ptrtoint [7 x i8]* @string to i32
  %interior = add i32 %base, 3
  %interiorPtr = inttoptr i32 %interior to i8*
  %char = load i8, i8* %interiorPtr, align 1
  %result = zext i8 %char to i32
  ret i32 %result
}

$ ./pnacl-sz -O2 -filetype=asm /tmp/test.ll
...
test:
.Ltest$__0:
	movzbl	string+3, %eax
	ret

Is this what you're looking for?

[tlively, 2016-07-27 18:32:23]

On 2016/07/27 13:28:18, stichnot wrote:
> On 2016/07/27 01:23:27, tlively wrote:
> > Is there any way to perform a load or store at an offset off of a global
> without
> > introducing an intermediary variable to store the address? Subzero seems to
> > support such an offset but I'm not sure what the bitcode would look like. If
> > this is possible I need to write a test with it.
> 
> You can't specify it directly in bitcode, but Subzero's address mode inference
> is what constructs the IR.  For example:
> 
> @string = internal global [7 x i8] c"abcdefg"
> define internal i32 @test() {
>   %base = ptrtoint [7 x i8]* @string to i32
>   %interior = add i32 %base, 3
>   %interiorPtr = inttoptr i32 %interior to i8*
>   %char = load i8, i8* %interiorPtr, align 1
>   %result = zext i8 %char to i32
>   ret i32 %result
> }
> 
> $ ./pnacl-sz -O2 -filetype=asm /tmp/test.ll
> ...
> test:
> .Ltest$__0:
> 	movzbl	string+3, %eax
> 	ret
> 
> Is this what you're looking for?

Since getting the offsets would require repeating part of address mode
inference, I added a TODO to come back to this later.

[tlively, 2016-07-27 18:32:36]

https://codereview.chromium.org/2183683003/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2183683003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:319: ConstantRelocatable *Reloc =
llvm::dyn_cast<ConstantRelocatable>(Op);
On 2016/07/27 13:19:34, stichnot wrote:
> auto *Reloc

Done.

https://codereview.chromium.org/2183683003/diff/1/src/IceASanInstrumentation.h
File src/IceASanInstrumentation.h (right):

https://codereview.chromium.org/2183683003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.h:38: : Instrumentation(Ctx), GlobalSizes(), RzNum(0)
{
On 2016/07/27 13:19:34, stichnot wrote:
> You probably don't need to explicitly specify the default initializer for
> GlobalSizes.

Done.

[Jim Stichnoth, 2016-07-27 22:32:31]

lgtm

[tlively, 2016-07-27 22:55:45]

Description was changed from

==========
Subzero: Removed unnecessary global access checks

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

to

==========
Subzero: Removed unnecessary global access checks

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[tlively, 2016-07-27 22:55:47]

Committed patchset #2 (id:20001) manually as
181a9bcb55b8d1e22210479e5210852d703855f5 (presubmit successful).