Improve error handling in the ARM integrated assembler.

Improve error handling in the ARM integrated assembler.

Up to now, all error handling in the ARM integrated assembler was
handled by forcing a text fixup. This CL tries to minimize the use of
fixup's to only be applied when there is an unimplemented form of an
instruction. All other cases now generate fatal error messages.

This CL should make it easier to determine what instructions still need
to be extended.

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

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

[Karl, 2015-12-03 20:33:57]

Description was changed from

==========
Improve error handling in the ARM integrated assembler.

Up to now, all error handling in the ARM integrated assembler was
handled by forcing a text fixup. This CL tries to minimize the use of
fixup's to only be applied when there is an unimplemented form of an
instruction. All other cases now generate fatal error messages.

This CL should make it easier to determine what instructions still need
to be extended.

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

to

==========
Improve error handling in the ARM integrated assembler.

Up to now, all error handling in the ARM integrated assembler was
handled by forcing a text fixup. This CL tries to minimize the use of
fixup's to only be applied when there is an unimplemented form of an
instruction. All other cases now generate fatal error messages.

This CL should make it easier to determine what instructions still need
to be extended.

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

[Karl, 2015-12-03 20:33:57]

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

[Jim Stichnoth, 2015-12-03 21:19:50]

https://codereview.chromium.org/1495093002/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1495093002/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:360: inline IValueT encodeRegister(const Operand
*OpReg, const char *RegName,
I don't think all the "inline" specifications are needed.

https://codereview.chromium.org/1495093002/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:371: if (!isGPRRegisterDefined(Reg))
What do you think about using BuildDefs to disable these verify*() checks in a
MINIMAL build?

https://codereview.chromium.org/1495093002/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:380: inline void verifyNot_NotPAndW(IValueT Address,
const char *InstName) {
The "Not_Not" substring, including the use of underscore, seems weird.

https://codereview.chromium.org/1495093002/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:876: constexpr const char *AdcName = "adc";
Too bad you can't easily reuse e.g. InstARM32Adc::Opcode ...

[Karl, 2015-12-03 23:52:59]

https://codereview.chromium.org/1495093002/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1495093002/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:360: inline IValueT encodeRegister(const Operand
*OpReg, const char *RegName,
On 2015/12/03 21:19:49, stichnot wrote:
> I don't think all the "inline" specifications are needed.

Removed inlines.

https://codereview.chromium.org/1495093002/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:371: if (!isGPRRegisterDefined(Reg))
On 2015/12/03 21:19:49, stichnot wrote:
> What do you think about using BuildDefs to disable these verify*() checks in a
> MINIMAL build?

I guess it is ok. Changing.

https://codereview.chromium.org/1495093002/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:380: inline void verifyNot_NotPAndW(IValueT Address,
const char *InstName) {
On 2015/12/03 21:19:49, stichnot wrote:
> The "Not_Not" substring, including the use of underscore, seems weird.

Renamed to verifyPOrNotW().

https://codereview.chromium.org/1495093002/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:876: constexpr const char *AdcName = "adc";
On 2015/12/03 21:19:50, stichnot wrote:
> Too bad you can't easily reuse e.g. InstARM32Adc::Opcode ...

Acknowledged.

[Jim Stichnoth, 2015-12-04 00:02:40]

otherwise lgtm

https://codereview.chromium.org/1495093002/diff/40001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1495093002/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:368: if (!BuildDefs::minimal() &&
!isGPRRegisterDefined(Reg))
I think you should follow the usual BuildDefs pattern:

  if (BuildDefs::minimal())
    return;
  if (!isGPRRegisterDefined(Reg))
    llvm::report_fatal_error(...);

[Karl, 2015-12-04 15:29:14]

Description was changed from

==========
Improve error handling in the ARM integrated assembler.

Up to now, all error handling in the ARM integrated assembler was
handled by forcing a text fixup. This CL tries to minimize the use of
fixup's to only be applied when there is an unimplemented form of an
instruction. All other cases now generate fatal error messages.

This CL should make it easier to determine what instructions still need
to be extended.

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

to

==========
Improve error handling in the ARM integrated assembler.

Up to now, all error handling in the ARM integrated assembler was
handled by forcing a text fixup. This CL tries to minimize the use of
fixup's to only be applied when there is an unimplemented form of an
instruction. All other cases now generate fatal error messages.

This CL should make it easier to determine what instructions still need
to be extended.

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

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

[Karl, 2015-12-04 15:29:17]

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

[Karl, 2015-12-04 15:29:30]

https://codereview.chromium.org/1495093002/diff/40001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1495093002/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:368: if (!BuildDefs::minimal() &&
!isGPRRegisterDefined(Reg))
On 2015/12/04 00:02:40, stichnot wrote:
> I think you should follow the usual BuildDefs pattern:
> 
>   if (BuildDefs::minimal())
>     return;
>   if (!isGPRRegisterDefined(Reg))
>     llvm::report_fatal_error(...);

Done.