New issue
Advanced search Search tips
Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017



Sign in to add a comment
link

Issue 24: Support shuffling of any operand type in Subzero

Reported by nicolasc...@google.com, Oct 28 2016 Project Member

Issue description

Subzero's instructions currently all accept Operand types (which can be a Variable or Constant) as source arguments, except for InstShuffleVector, which expects Variables. This prevents us from using Operand as the implementation type for the Reactor Value class. This results in requiring assign operations to convert Constants into Variables, affecting code quality. We could implement a constant propagation optimization pass to deal with that, but it would be more efficient to make the InstShuffleVector source arguments Operands and avoid the assign operations altogether.

Note that lowerShuffleVector() currently immediately casts the arguments to Variables, so all of the targets will have to be made to support any Operand type. For targets we don't currently care about this could be handled using assign operations.
 

Comment 1 by bugdroid1@chromium.org, Dec 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/native_client/pnacl-subzero/+/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86

commit 579b1b3a84da15d233c9ab4e3d3dc35cff4edf86
Author: Nicolas Capens <capn@google.com>
Date: Fri Dec 09 19:56:03 2016

Generalize vector shuffling to accept any operand.

The arguments get legalized to Reg or Mem, so we can allow constants
as well (including undef values). This change makes all instruction's
source arguments Ice::Operands.

BUG= swiftshader:24 

Change-Id: I1659cdfdb1b8a12c4acc7c473211d8a67bfd5868
Reviewed-on: https://chromium-review.googlesource.com/418504
Reviewed-by: Jim Stichnoth <stichnot@chromium.org>

[modify] https://crrev.com/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86/src/IceTargetLoweringX86Base.h
[modify] https://crrev.com/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86/src/IceTargetLoweringX86BaseImpl.h
[modify] https://crrev.com/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86/src/IceInst.cpp
[modify] https://crrev.com/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86/src/IceInst.h
[modify] https://crrev.com/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86/src/IceTargetLoweringARM32.cpp

Comment 2 by bugdroid1@chromium.org, Dec 10 2016

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/15060bb012724757c3944f55da9fd668af78e559

commit 15060bb012724757c3944f55da9fd668af78e559
Author: Nicolas Capens <capn@google.com>
Date: Tue Dec 06 03:17:19 2016

Eliminate assign operations for constants.

We were using Ice::Variable as the implementation type for Reactor's
abstract Value class only because shuffle operations required them.

 Bug swiftshader:24 

Change-Id: If87ab5f0b0bca5fbffe2df250ed03b7a1b1490c6
Reviewed-on: https://swiftshader-review.googlesource.com/8258
Reviewed-by: Nicolas Capens <capn@google.com>
Tested-by: Nicolas Capens <capn@google.com>

[modify] https://crrev.com/15060bb012724757c3944f55da9fd668af78e559/src/Reactor/SubzeroReactor.cpp
[modify] https://crrev.com/15060bb012724757c3944f55da9fd668af78e559/third_party/pnacl-subzero

Comment 3 by bugdroid1@chromium.org, Jan 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/native_client/src/native_client.git/+/01d3ae00d277726b8aaee572120a5ba2a31d511e

commit 01d3ae00d277726b8aaee572120a5ba2a31d511e
Author: Jim Stichnoth <stichnot@chromium.org>
Date: Wed Jan 04 07:04:41 2017

PNaCl: Update subzero revision in pnacl/COMPONENT_REVISIONS

This pulls in the following Subzero changes:

ec92917: (jaydeep.patil@imgtec.com) [SubZero] Legalize load, store for MIPS post lower
4683237: (sagar.thakur@imgtec.com) [Subzero][MIPS32] Fix alloca alignment and offset for Om1 and O2 optimization
45e4d5e: (jaydeep.patil@imgtec.com) [SubZero] Handle relocatable constants for MIPS
3a01f33: (jaydeep.patil@imgtec.com) [SubZero] Implement Fcmp, ICmp, Cast and Select for vector type
f52cea4: (capn@google.com) Fix unpacking from a single vector.
7638e27: (capn@google.com) Add x86 vector packing instructions.
0e90622: (capn@google.com) Generate error on unexpected intrisics.
89be887: (sagar.thakur@imgtec.com) [Subzero][MIPS32] Account for variable alloca alignment bytes in addProlog
1448d95: (capn@google.com) Optimize shuffles corresponding to x86 punpckh instructions.
7145e69: (stichnot@chromium.org) Subzero: Fix compiler warnings.
32f9cce: (capn@google.com) Fix 64-bit pointer type for non-x32 ABIs.
ef8210d: (capn@google.com) Implement vector packing intrinsics.
8b8af82: (capn@google.com) Implement bitcast between i32 and (emulated) v4i8.
e3cabda: (capn@google.com) Implement vector sign mask intrinsic.
d0e3030: (capn@google.com) Assert that PNaCl bitcode only uses 128-bit vector casts.
c9e91af: (capn@google.com) Remove verified asserts.
61593fb: (capn@google.com) Fix unit tests.
a3688ea: (capn@google.com) Fix two-vector unpack case.
a7979bf: (jaydeep.patil@imgtec.com) [SubZero] Fix f64 to/from i64 moves
0dabe18: (makdstefan@gmail.com) Subzero, MIPS32: Remove --skip-unimplemented from lit tests
130aca7: (jaydeep.patil@imgtec.com) [SubZero] Generate relocations for MIPS
73ae4fd: (capn@google.com) Preserve rsi and rdi when using Microsoft x86-64 calling convention.
7ad028e: (makdstefan@gmail.com) This patch enables running a couple more of lit tests for MIPS32
956cfd6: (capn@google.com) Generalize the Sqrt intrinsic to process vectors.
13cde0f: (capn@google.com) Implement integer vector multiply intrinsics.
67a49b5: (capn@google.com) Implement saturated vector add/subtract.
0c4c07d: (jaydeep.patil@imgtec.com) [SubZero] Fix code generation for vector type
f8c9977: (makdstefan@gmail.com) Subzero, MIPS32: Stacksave/Stackrestore implementation
f0d12c3: (capn@google.com) Implement floating-point rounding intrinsic.
3da9f65: (jaydeep.patil@imgtec.com) [SubZero] Generate MIPS.abiflags section
21f78bb: (jaydeep.patil@imgtec.com) [SubZero] Utilize instructions with immediate operands
83425de: (capn@google.com) Support 64-bit jump tables with LP64 data model.
6e03343: (makdstefan@gmail.com) Subzero, MIPS32: Sandbox initial patch
becb85f: (sagar.thakur@imgtec.com) [Subzero][MIPS] Implements atomic intrinsics for MIPS32
8208e75: (makdstefan@gmail.com) Subzero, MIPS32: Changes for improving sandbox crosstest results
2220990: (capn@google.com) Fix offset adjustment in x86 address optimization.
ef18fc5: (capn@google.com) Match sub-vector load/store operand order to regular load/store.
8be6975: (jaydeep.patil@imgtec.com) [SubZero] Fix size of arguments on stack
e1e1783: (capn@google.com) Fix skipping deleted instructions before replacing operands.
a29da90: (capn@google.com) Ensure that the sub-vector load destination is a register.
579b1b3: (capn@google.com) Generalize vector shuffling to accept any operand.
373913f: (stichnot@chromium.org) Subzero: Legalize the movzx argument.
35bbca3: (stichnot@chromium.org) Subzero: Fix multiply defined symbols in Windows/g++ build.

BUG=  swiftshader:24 
BUG=  swiftshader:9 
BUG= swiftshader:22
BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4384
BUG=  swiftshader:15 
TEST= PNaCl toolchain trybots
R=dschuff@chromium.org

Review-Url: https://codereview.chromium.org/2602613002 .

[modify] https://crrev.com/01d3ae00d277726b8aaee572120a5ba2a31d511e/pnacl/COMPONENT_REVISIONS

Comment 4 by bugdroid1@chromium.org, Jan 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/native_client/src/native_client.git/+/026b76327c0c443fcf98064e315ca398c4b544d1

commit 026b76327c0c443fcf98064e315ca398c4b544d1
Author: Jim Stichnoth <stichnot@chromium.org>
Date: Mon Jan 09 22:42:11 2017

Update revision for PNaCl

Update f7d719122cd7c2fe3ebc52e7c0b011c583bf3e9c -> 5dfe030a71ca66e72c5719ef5034c2ed24706c43

Pull the following PNaCl changes into NaCl:
  a123a8b: (stichnot@chromium.org) PNaCl: Update subzero revision in pnacl/COMPONENT_REVISIONS
    | 8fbddc6: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Randomly insert NOP
    | 3e37647: (capn@google.com) Abstract the ELFStreamer class.
    | 40fc819: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: nacl-other-intrinsics-mips merged to original file
    | 132ea7a: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Floating point support in ELF output
    | 41ce47c: (capn@google.com) Allow 64-bit code to be stored as ELF64.
    | 464df5b: (capn@google.com) Implement Microsoft x86-64 calling convention support.
    | cc6dea7: (jaydeep.patil@imgtec.com) [SubZero] Use DIV instruction instead of TargetHelperCall
    | 3b61d70: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: lowerUnreachable
    | cf9c12f: (jaydeep.patil@imgtec.com) [SubZero] lower float and double constants for MIPS
    | 8d16c1d: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Encoding of FP comparison instructions
    | 0465d02: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Intrinsic call Trap
    | d895447: (jaydeep.patil@imgtec.com) [SubZero] Fix floating-point comparison for MIPS
    | f5d8e09: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Remove duplicate functionalities
    | cadda79: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Instruction NOR, pseudoinstruction NOT
    | d27ce3d: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Intrinsic call Ctlz for i32
    | 6ee373f: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Fix floating point comparison crosstest
    | 0a7f99d: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Intrinsic call Cttz for i32
    | 86b60ef: (sagar.thakur@imgtec.com) [Subzero][MIPS32] Implements 64-bit shl, lshr, ashr for MIPS
    | 98405d3: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: lowerSelect for i64
    | 623f8ce: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Cross-testing enabled for MIPS32
    | 4c49b10: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Filling missing bits from genTargetHelperCallFor
    | 58eeedf: (jaydeep.patil@imgtec.com) Subzero, MIPS32: Binding intrablock labels, unconditional branch
    | 6fd9c0e: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Intrinsic calls Ctlz and Cttz for i64
    | 175cb13: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: Intrinsic call Bswap for i16, i32 and i64
    | 6163c62: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: DIVU instruction encoding
    | b0f09fc: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: MOVZ instruction encoding
    | 70b6ed4: (sagar.thakur@imgtec.com) [Subzero][MIPS] Implement 64-bit integer compare operations
    | fe93fdd: (Srdjan.Obucina@imgtec.com) Subzero, MIPS32: SRAV instruction encoding
    | f53580b: (capn@google.com) Don't emit address size prefixes for native x86-64 ABI.
    | 269eed4: (sagar.thakur@imgtec.com) [Subzero][MIPS] Add RUN command line with -Om1 in test 64bit.pnacl.ll
    | 9309756: (sagar.thakur@imgtec.com) [Subzero][MIPS] Implement conditional branches with 64-bit integer compares
    | 958ddb7: (jaydeep.patil@imgtec.com) [SubZero] Vector types support for MIPS
    | 033dda7: (stichnot@chromium.org) Subzero: Remove --skip-unimplemented from ARM lit tests.
    | afe5fe2: (makdstefan@gmail.com) Subzero, MIPS32: Fix conditional mov instructions
    | 533a514: (stichnot@chromium.org) Subzero: Fix "make -f Makefile.standalone check-lit FORCEASM=1".
    | acfb3df: (capn@google.com) Implement intrinsics for loading/storing subvectors.
    | 71c6937: (capn@google.com) Optimize lowering of x86 byte and word vector unpack.
    | b093539: (capn@google.com) Optimize x86 vector shift by constant.
    | 46f4fea: (capn@google.com) Support running unit tests on Windows.
    | b001cc4: (sagar.thakur@imgtec.com) [Subzero][MIPS32] Implement bitcast operation for both 32-bit and 64-bit operands
  d564ff3: (petarj@mips.com) PNaCl: Update llvm revision in pnacl/COMPONENT_REVISIONS
    | 5a87509: (kschimpf@google.com) Create one-off tool pnacl-hack-memset
    | 1d79adf: (petar.jovanovic@rt-rk.com) Cherry-pick r237153: [Mips] Return false for isFPCloseToIncomingSP()
    | 7251d5b: (petar.jovanovic@rt-rk.com) Cherry-pick r246309: [mips] Remove incorrect DebugLoc entries from prologue
  cb27d1f: (petarj@mips.com) [MIPS] Remove explicit masks from assembly code
  01d3ae0: (stichnot@chromium.org) PNaCl: Update subzero revision in pnacl/COMPONENT_REVISIONS
    | ec92917: (jaydeep.patil@imgtec.com) [SubZero] Legalize load, store for MIPS post lower
    | 4683237: (sagar.thakur@imgtec.com) [Subzero][MIPS32] Fix alloca alignment and offset for Om1 and O2 optimization
    | 45e4d5e: (jaydeep.patil@imgtec.com) [SubZero] Handle relocatable constants for MIPS
    | 3a01f33: (jaydeep.patil@imgtec.com) [SubZero] Implement Fcmp, ICmp, Cast and Select for vector type
    | f52cea4: (capn@google.com) Fix unpacking from a single vector.
    | 7638e27: (capn@google.com) Add x86 vector packing instructions.
    | 0e90622: (capn@google.com) Generate error on unexpected intrisics.
    | 89be887: (sagar.thakur@imgtec.com) [Subzero][MIPS32] Account for variable alloca alignment bytes in addProlog
    | 1448d95: (capn@google.com) Optimize shuffles corresponding to x86 punpckh instructions.
    | 7145e69: (stichnot@chromium.org) Subzero: Fix compiler warnings.
    | 32f9cce: (capn@google.com) Fix 64-bit pointer type for non-x32 ABIs.
    | ef8210d: (capn@google.com) Implement vector packing intrinsics.
    | 8b8af82: (capn@google.com) Implement bitcast between i32 and (emulated) v4i8.
    | e3cabda: (capn@google.com) Implement vector sign mask intrinsic.
    | d0e3030: (capn@google.com) Assert that PNaCl bitcode only uses 128-bit vector casts.
    | c9e91af: (capn@google.com) Remove verified asserts.
    | 61593fb: (capn@google.com) Fix unit tests.
    | a3688ea: (capn@google.com) Fix two-vector unpack case.
    | a7979bf: (jaydeep.patil@imgtec.com) [SubZero] Fix f64 to/from i64 moves
    | 0dabe18: (makdstefan@gmail.com) Subzero, MIPS32: Remove --skip-unimplemented from lit tests
    | 130aca7: (jaydeep.patil@imgtec.com) [SubZero] Generate relocations for MIPS
    | 73ae4fd: (capn@google.com) Preserve rsi and rdi when using Microsoft x86-64 calling convention.
    | 7ad028e: (makdstefan@gmail.com) This patch enables running a couple more of lit tests for MIPS32
    | 956cfd6: (capn@google.com) Generalize the Sqrt intrinsic to process vectors.
    | 13cde0f: (capn@google.com) Implement integer vector multiply intrinsics.
    | 67a49b5: (capn@google.com) Implement saturated vector add/subtract.
    | 0c4c07d: (jaydeep.patil@imgtec.com) [SubZero] Fix code generation for vector type
    | f8c9977: (makdstefan@gmail.com) Subzero, MIPS32: Stacksave/Stackrestore implementation
    | f0d12c3: (capn@google.com) Implement floating-point rounding intrinsic.
    | 3da9f65: (jaydeep.patil@imgtec.com) [SubZero] Generate MIPS.abiflags section
    | 21f78bb: (jaydeep.patil@imgtec.com) [SubZero] Utilize instructions with immediate operands
    | 83425de: (capn@google.com) Support 64-bit jump tables with LP64 data model.
    | 6e03343: (makdstefan@gmail.com) Subzero, MIPS32: Sandbox initial patch
    | becb85f: (sagar.thakur@imgtec.com) [Subzero][MIPS] Implements atomic intrinsics for MIPS32
    | 8208e75: (makdstefan@gmail.com) Subzero, MIPS32: Changes for improving sandbox crosstest results
    | 2220990: (capn@google.com) Fix offset adjustment in x86 address optimization.
    | ef18fc5: (capn@google.com) Match sub-vector load/store operand order to regular load/store.
    | 8be6975: (jaydeep.patil@imgtec.com) [SubZero] Fix size of arguments on stack
    | e1e1783: (capn@google.com) Fix skipping deleted instructions before replacing operands.
    | a29da90: (capn@google.com) Ensure that the sub-vector load destination is a register.
    | 579b1b3: (capn@google.com) Generalize vector shuffling to accept any operand.
    | 373913f: (stichnot@chromium.org) Subzero: Legalize the movzx argument.
    | 35bbca3: (stichnot@chromium.org) Subzero: Fix multiply defined symbols in Windows/g++ build.
  5dfe030: (stichnot@chromium.org) PNaCl: Update subzero revision in pnacl/COMPONENT_REVISIONS

BUG= <none>
BUG= none
BUG= pnacl-llc aborts on run_vector_extension_test for MIPS
BUG=  swiftshader:15 
BUG=  swiftshader:24 
R=dschuff@chromium.org, petarj@mips.com, stichnot@chromium.org
TEST=git cl try
(Please LGTM this change and tick the "commit" box)

Review-Url: https://codereview.chromium.org/2623603002 .

[modify] https://crrev.com/026b76327c0c443fcf98064e315ca398c4b544d1/toolchain_revisions/pnacl_newlib.json
[modify] https://crrev.com/026b76327c0c443fcf98064e315ca398c4b544d1/toolchain_revisions/pnacl_newlib_raw.json
[modify] https://crrev.com/026b76327c0c443fcf98064e315ca398c4b544d1/toolchain_revisions/pnacl_translator.json

Comment 5 by nicolasc...@google.com, Jan 13 2017

Project Member
Status: Fixed (was: Accepted)

Comment 6 by bugdroid1@chromium.org, May 3 2017

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86

commit 579b1b3a84da15d233c9ab4e3d3dc35cff4edf86
Author: Nicolas Capens <capn@google.com>
Date: Fri Dec 09 22:29:22 2016

Generalize vector shuffling to accept any operand.

The arguments get legalized to Reg or Mem, so we can allow constants
as well (including undef values). This change makes all instruction's
source arguments Ice::Operands.

BUG= swiftshader:24 

Change-Id: I1659cdfdb1b8a12c4acc7c473211d8a67bfd5868
Reviewed-on: https://chromium-review.googlesource.com/418504
Reviewed-by: Jim Stichnoth <stichnot@chromium.org>

[modify] https://crrev.com/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86/src/IceInst.cpp
[modify] https://crrev.com/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86/src/IceInst.h
[modify] https://crrev.com/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86/src/IceTargetLoweringARM32.cpp
[modify] https://crrev.com/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86/src/IceTargetLoweringX86Base.h
[modify] https://crrev.com/579b1b3a84da15d233c9ab4e3d3dc35cff4edf86/src/IceTargetLoweringX86BaseImpl.h

Sign in to add a comment