Subzero: Support multiple fixups in one instruction.

Subzero: Support multiple fixups in one instruction.

The integrated assembler assumed there is at most one fixup per instruction.  For x86, there could actually be two fixups, for any instruction that allows memory and immediate operands at the same time.  Using the now-default -build-on-read flag, it happens in spec2k - the smallest function I found where this happens is 176.gcc and perm_tree_cons.

This changes the textual emission of integrated assembler code to allow for multiple consecutive fixups in a single instruction.

BUG= none
R=jvoung@chromium.org

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

[Jim Stichnoth, 2014-11-02 17:47:41]

stichnot@chromium.org changed reviewers:
+ jvoung@chromium.org

[Jim Stichnoth, 2014-11-02 17:49:21]

https://codereview.chromium.org/693393002/diff/1/tests_lit/llvm2ice_tests/ias...
File tests_lit/llvm2ice_tests/ias-multi-reloc.ll (right):

https://codereview.chromium.org/693393002/diff/1/tests_lit/llvm2ice_tests/ias...
tests_lit/llvm2ice_tests/ias-multi-reloc.ll:10: define internal void
@store_immediate_to_global() {
This is the only test that actually triggered the bug.  The other two tests are
forward-looking for optimizations that O2 might/should do.

[jvoung (off chromium), 2014-11-03 17:18:56]

https://codereview.chromium.org/693393002/diff/1/src/assembler.cpp
File src/assembler.cpp (right):

https://codereview.chromium.org/693393002/diff/1/src/assembler.cpp#newcode78
src/assembler.cpp:78: // there is none.  Assumes were added in increasing order.
"Assumes fixups were added in..."

https://codereview.chromium.org/693393002/diff/1/tests_lit/llvm2ice_tests/ias...
File tests_lit/llvm2ice_tests/ias-multi-reloc.ll (right):

https://codereview.chromium.org/693393002/diff/1/tests_lit/llvm2ice_tests/ias...
tests_lit/llvm2ice_tests/ias-multi-reloc.ll:4: ; RUN: %p2i -i %s --args -O2
-ias=1 | FileCheck %s
nit: the other integrated assembler tests don't use "-ias=1"

https://codereview.chromium.org/693393002/diff/1/tests_lit/llvm2ice_tests/ias...
tests_lit/llvm2ice_tests/ias-multi-reloc.ll:32: ; CHECK: .long p_global_int
Hmm, I would have thought that "global_int" is the "displacement" here and comes
earlier, since it's used as the i32* for the load/store, and "p_global_int" is
the "immediate" being added to the existing value and comes later?

Oh, is the issue that it's not fused right now, so global_int shows up multiple
times and either order is statisfied?

Can the role of p_global_int and global_int be swapped to be consistent with the
other test cases?

[Jim Stichnoth, 2014-11-03 19:35:50]

https://codereview.chromium.org/693393002/diff/1/src/assembler.cpp
File src/assembler.cpp (right):

https://codereview.chromium.org/693393002/diff/1/src/assembler.cpp#newcode78
src/assembler.cpp:78: // there is none.  Assumes were added in increasing order.
On 2014/11/03 17:18:55, jvoung wrote:
> "Assumes fixups were added in..."

Done.

https://codereview.chromium.org/693393002/diff/1/tests_lit/llvm2ice_tests/ias...
File tests_lit/llvm2ice_tests/ias-multi-reloc.ll (right):

https://codereview.chromium.org/693393002/diff/1/tests_lit/llvm2ice_tests/ias...
tests_lit/llvm2ice_tests/ias-multi-reloc.ll:4: ; RUN: %p2i -i %s --args -O2
-ias=1 | FileCheck %s
On 2014/11/03 17:18:55, jvoung wrote:
> nit: the other integrated assembler tests don't use "-ias=1"

OK, removed for consistency.

https://codereview.chromium.org/693393002/diff/1/tests_lit/llvm2ice_tests/ias...
tests_lit/llvm2ice_tests/ias-multi-reloc.ll:32: ; CHECK: .long p_global_int
On 2014/11/03 17:18:55, jvoung wrote:
> Hmm, I would have thought that "global_int" is the "displacement" here and
comes
> earlier, since it's used as the i32* for the load/store, and "p_global_int" is
> the "immediate" being added to the existing value and comes later?
> 
> Oh, is the issue that it's not fused right now, so global_int shows up
multiple
> times and either order is statisfied?
> 
> Can the role of p_global_int and global_int be swapped to be consistent with
the
> other test cases?

I swapped the order to be consistent.  This also meant changing the int types to
char, to avoid multiplying the relocatable immediate value by 4.

When fusing is actually implemented for these two cases, I think we'll need to
swap the order of the CHECK lines.

[jvoung (off chromium), 2014-11-03 19:50:12]

LGTM

[Jim Stichnoth, 2014-11-03 20:36:33]

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