Implements include/exclude register lists for translation.

Implements include/exclude register lists for translation.

This allows better debugging of register encodings into
instructions, in the integrated assembler.

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=5403f5dc6b43f077bbbee369bbe178b660654366

[Karl, 2016-01-08 23:42:21]

Description was changed from

==========
Implements include/exclude register lists for ARM translation.

This allows better debugging of register encodings into ARM
instructions, in the integrated assembler.

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

to

==========
Implements include/exclude register lists for ARM translation.

This allows better debugging of register encodings into ARM
instructions, in the integrated assembler.

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

[Karl, 2016-01-08 23:42:21]

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

[Jim Stichnoth, 2016-01-10 16:51:26]

I'm "happy" to report that building 176.gcc this way:

./pydir/szbuild_spec2k.py -v -O2 --target=arm32 --filetype=asm --force --run
176.gcc --sz=--use-registers=r2,r0,r3,r6,r7

causes the generated code to crash. :)  Same behavior with Om1 so it's probably
an issue with basic lowering or more likely register alias management, but that
can be addressed later.

https://codereview.chromium.org/1571433004/diff/20001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1571433004/diff/20001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:292: "use-registers",
Can you add the cl::CommaSeparated option here, which should eliminate the need
for custom comma parsing?  See the --verbose option below for reference.

https://codereview.chromium.org/1571433004/diff/20001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:298: ExcludedRegisters("exclude-registers",
I would give these two option names a common prefix, like -reg-include and
-reg-exclude, so that they show up together in the -help output.

https://codereview.chromium.org/1571433004/diff/20001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1571433004/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:412: /// see
anonymous_namespace{IceClFlags.cpp}::UxcludedRegisters;
ExcludedRegisters

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1838: llvm::SmallBitVector
GPKeepRegisters(RegARM32::Reg_NUM);
I think it might not be too hard to generalize this so that it's usable for
other architectures.

You can define a target-specific function that maps a register to a numeric
register class, such as:
  IsGPR ? 0 : IsFP32 ? 1 : IsFP64 ? 2 : IsVec128 ? 3 : 4
or:
  (IsGPR << 0) | (IsFP32 << 1) | (IsFP64 << 2) | (IsVec128 << 3)

Then either a std::vector<llvm::SmallBitVector> indexed by register class, or a
std::unordered_map<uint32_t, llvm::SmallBitVector> indexed by register class.

This target-specific mapping function could be passed in as a lambda.

In this model, probably the sense of e.g. NotGPRegisters has to change to
AllGPRegisters, and then
  Registers &= NotGPRegisters;
gets changed to
  Registers &= ~GPRegisters;
or the more efficient
  Registers.reset(GPRegisters);

I haven't worked this through completely so there's undoubtedly some yuckiness
lurking in trying to make such a general function properly templatized, or
otherwise with target-neutral arguments and data structures.  But it would be
great to have an initial implementation that doesn't call out 4 specific
register classes to prepare for extending this to x86.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1845: llvm::SmallBitVector
NotVectorRegisters(RegARM32::Reg_NUM);
Do we need IsI64Pair as well?  If so, there are some command line parsing issues
because those "register" names contain a comma...

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1878: const std::unordered_set<std::string>
Excluded) {
&Excluded

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1883: Registers[i] = 0;
Registers[i] = false;

Might as well remove the "if (Registers[i])" condition as well.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1914: const std::unordered_set<std::string>
&RestrictedUses =
A problem with calculating the restrictions here is that getRegisterSet is
called at least 2-4 times per translated method, and the restrictions never
change during translation.  (Fortunately the empty() calls probably keep it fast
for the common case.)

I think it would be better for staticInit() to calculate a mask which is ANDed
with Registers before returning.

https://codereview.chromium.org/1571433004/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/vadd.ll (right):

https://codereview.chromium.org/1571433004/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/vadd.ll:61: ; ASM-NEXT:     # [sp, #8] = def.pseudo
Trailing whitespace here and 3 other places.

Actually it turns out that's just the tip of the iceberg:

  git grep ' $'

Fix the new ones here, and the rest in a separate CL.

https://codereview.chromium.org/1571433004/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/vadd.ll:73: ; DIS-NEXT:   14:	ed9daa02
untabify

[John, 2016-01-11 15:43:26]

https://codereview.chromium.org/1571433004/diff/20001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1571433004/diff/20001/src/IceDefs.h#newcode49
src/IceDefs.h:49: #include <unordered_set>
Please don't add this include if you don't need it. Add this #include in the
files it is needed (preferably .cpp files)

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1845: llvm::SmallBitVector
NotVectorRegisters(RegARM32::Reg_NUM);
On 2016/01/10 16:51:25, stichnot wrote:
> Do we need IsI64Pair as well?  If so, there are some command line parsing
issues
> because those "register" names contain a comma...

I64 pairs should probably be turned off by disabling their individual parts --
just my 2cc.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1848: NotGPRegisters[i] = !Entry.IsGPR;
Register pairs are not being handled here at all. As a matter of fact, any
register type that is not GPR, FP32, FP64, or Vec, are not being handled here
(they are incidentally because the Is??? features are mutually exclusive, but I
don't think depending on that is a good idea.)

optional: You could have something like



bool canBeUsed(int Register, const unordered_set<> &RestrictedRegisters) {
  return RestrictedUses.empty() || RestrictedUses.count(i) > 0;
}

bool isOther(int i) {
  const auto Register = RegARM32::Table[i];
  return !(Register.IsGPR | Register.IsFP32 || Register.IsFP64 ||
Register.IsVec128);
}

for (size_t i = 0; i < RegARM32::Reg_NUM; ++i) {
  SRegisterMask[i] =
     (IsFP32 && canBeUsed(i)) || IsFP64 || IsVec128 || IsGPR || isOther(i);
  DRegisterMask[i] =
     IsFP32 || (IsFP64 && canBeUsed(i)) || IsVec128 || IsGPR || isOther(i);
  QRegisterMask[i] =
     IsFP32 || IsFP64 || (IsVec128 && canBeUsed(i)) || IsGPR || isOther(i);
  GPRegisterMask[i] =
     IsFP32 || IsFP64 || IsVec128 || (IsGPR && canBeUsed(i)) || isOther(i);
}

Registers &=
    GPRegisterMask & SRegisterMask & DRegisterMask & QRegisterMask;

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1877: void
applyExcludedRegisters(llvm::SmallBitVector &Registers,
pass registers by pointer (i.e., SmallBitVector *)

In the arm lowering I am trying to remove (and have already removed) non-const
references.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1881: if (Excluded.find(Entry.Name) !=
Excluded.end()) {
nit: this is less efficient than

Excluded.count(Entry.Name) > 0

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1894: for (size_t i = 0; i < RegARM32::Reg_NUM;
++i) {
why? just curious...

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1914: const std::unordered_set<std::string>
&RestrictedUses =
s/RestrictedUses/RestrictedRegisters
s/ExcludedUses/ExcludedRegisters

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1919: const std::unordered_set<std::string>
&ExcludedUses =
I **really** wish we used `auto' here and above.

[Karl, 2016-01-12 23:44:05]

https://codereview.chromium.org/1571433004/diff/20001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1571433004/diff/20001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:292: "use-registers",
On 2016/01/10 16:51:25, stichnot wrote:
> Can you add the cl::CommaSeparated option here, which should eliminate the
need
> for custom comma parsing?  See the --verbose option below for reference.

Done.

https://codereview.chromium.org/1571433004/diff/20001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:298: ExcludedRegisters("exclude-registers",
On 2016/01/10 16:51:25, stichnot wrote:
> I would give these two option names a common prefix, like -reg-include and
> -reg-exclude, so that they show up together in the -help output.

Switched to '-reg-use' and '-reg-exclude'.

https://codereview.chromium.org/1571433004/diff/20001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1571433004/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:412: /// see
anonymous_namespace{IceClFlags.cpp}::UxcludedRegisters;
On 2016/01/10 16:51:25, stichnot wrote:
> ExcludedRegisters

Done.

https://codereview.chromium.org/1571433004/diff/20001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1571433004/diff/20001/src/IceDefs.h#newcode49
src/IceDefs.h:49: #include <unordered_set>
On 2016/01/11 15:43:26, John wrote:
> Please don't add this include if you don't need it. Add this #include in the
> files it is needed (preferably .cpp files)

I need it in IceClFlags.h. Following the lead of other such collections in
header files, I added it here.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1838: llvm::SmallBitVector
GPKeepRegisters(RegARM32::Reg_NUM);
On 2016/01/10 16:51:25, stichnot wrote:
> I think it might not be too hard to generalize this so that it's usable for
> other architectures.
> 
> You can define a target-specific function that maps a register to a numeric
> register class, such as:
>   IsGPR ? 0 : IsFP32 ? 1 : IsFP64 ? 2 : IsVec128 ? 3 : 4
> or:
>   (IsGPR << 0) | (IsFP32 << 1) | (IsFP64 << 2) | (IsVec128 << 3)
> 
> Then either a std::vector<llvm::SmallBitVector> indexed by register class, or
a
> std::unordered_map<uint32_t, llvm::SmallBitVector> indexed by register class.
> 
> This target-specific mapping function could be passed in as a lambda.
> 
> In this model, probably the sense of e.g. NotGPRegisters has to change to
> AllGPRegisters, and then
>   Registers &= NotGPRegisters;
> gets changed to
>   Registers &= ~GPRegisters;
> or the more efficient
>   Registers.reset(GPRegisters);
> 
> I haven't worked this through completely so there's undoubtedly some yuckiness
> lurking in trying to make such a general function properly templatized, or
> otherwise with target-neutral arguments and data structures.  But it would be
> great to have an initial implementation that doesn't call out 4 specific
> register classes to prepare for extending this to x86.

Ok. Refactored the code to put this function in IceTargetLowering.{h,cpp} using
name getRegisterSetMask(). Then modified TargetARM32::staticInit() to call this
to build the set.

Also added to getRegisterSetMask() code to trace the available registers if the
corresponding verbose option is given.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1845: llvm::SmallBitVector
NotVectorRegisters(RegARM32::Reg_NUM);
On 2016/01/10 16:51:25, stichnot wrote:
> Do we need IsI64Pair as well?  If so, there are some command line parsing
issues
> because those "register" names contain a comma...

Kept the register pairs, so that all registers can be defined. Fixed comma issue
by adding "RegNameFcn" that replace ", " with ":" in register names. This solves
the problem of how to specify on the command line.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1848: NotGPRegisters[i] = !Entry.IsGPR;
On 2016/01/11 15:43:26, John wrote:
> Register pairs are not being handled here at all. As a matter of fact, any
> register type that is not GPR, FP32, FP64, or Vec, are not being handled here
> (they are incidentally because the Is??? features are mutually exclusive, but
I
> don't think depending on that is a good idea.)
> 
> optional: You could have something like
> 
> 
> 
> bool canBeUsed(int Register, const unordered_set<> &RestrictedRegisters) {
>   return RestrictedUses.empty() || RestrictedUses.count(i) > 0;
> }
> 
> bool isOther(int i) {
>   const auto Register = RegARM32::Table[i];
>   return !(Register.IsGPR | Register.IsFP32 || Register.IsFP64 ||
> Register.IsVec128);
> }
> 
> for (size_t i = 0; i < RegARM32::Reg_NUM; ++i) {
>   SRegisterMask[i] =
>      (IsFP32 && canBeUsed(i)) || IsFP64 || IsVec128 || IsGPR || isOther(i);
>   DRegisterMask[i] =
>      IsFP32 || (IsFP64 && canBeUsed(i)) || IsVec128 || IsGPR || isOther(i);
>   QRegisterMask[i] =
>      IsFP32 || IsFP64 || (IsVec128 && canBeUsed(i)) || IsGPR || isOther(i);
>   GPRegisterMask[i] =
>      IsFP32 || IsFP64 || IsVec128 || (IsGPR && canBeUsed(i)) || isOther(i);
> }
> 
> Registers &=
>     GPRegisterMask & SRegisterMask & DRegisterMask & QRegisterMask;

Fixed this by adding enum "RegClass" and static method getRegClass() to
IceRegistersARM32.h. This is then passed to the command line processor to
categorize what classes each register is in.  I added a "RegClass_Other" for any
register not explicitly defined in some other register class.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1877: void
applyExcludedRegisters(llvm::SmallBitVector &Registers,
On 2016/01/11 15:43:26, John wrote:
> pass registers by pointer (i.e., SmallBitVector *)
> 
> In the arm lowering I am trying to remove (and have already removed) non-const
> references.

Removed. Now refactored into Ice::TargetLowering::getRegisterSetMask().

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1878: const std::unordered_set<std::string>
Excluded) {
On 2016/01/10 16:51:26, stichnot wrote:
> &Excluded

No longer applicable.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1881: if (Excluded.find(Entry.Name) !=
Excluded.end()) {
On 2016/01/11 15:43:26, John wrote:
> nit: this is less efficient than
> 
> Excluded.count(Entry.Name) > 0

Changed code to use method count rather than find.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1883: Registers[i] = 0;
On 2016/01/10 16:51:25, stichnot wrote:
> Registers[i] = false;
> 
> Might as well remove the "if (Registers[i])" condition as well.

Acknowledged.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1894: for (size_t i = 0; i < RegARM32::Reg_NUM;
++i) {
On 2016/01/11 15:43:26, John wrote:
> why? just curious...

Actually, I probably should have used SizeT. I did this because the index is
used to access arrays, and the type isn't int.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1914: const std::unordered_set<std::string>
&RestrictedUses =
On 2016/01/10 16:51:25, stichnot wrote:
> A problem with calculating the restrictions here is that getRegisterSet is
> called at least 2-4 times per translated method, and the restrictions never
> change during translation.  (Fortunately the empty() calls probably keep it
fast
> for the common case.)
> 
> I think it would be better for staticInit() to calculate a mask which is ANDed
> with Registers before returning.

Factored out into staticInit() that sets static field RegisterSetMask.

https://codereview.chromium.org/1571433004/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1919: const std::unordered_set<std::string>
&ExcludedUses =
On 2016/01/11 15:43:26, John wrote:
> I **really** wish we used `auto' here and above.

Acknowledged.

https://codereview.chromium.org/1571433004/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/vadd.ll (right):

https://codereview.chromium.org/1571433004/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/vadd.ll:61: ; ASM-NEXT:     # [sp, #8] = def.pseudo
On 2016/01/10 16:51:26, stichnot wrote:
> Trailing whitespace here and 3 other places.
> 
> Actually it turns out that's just the tip of the iceberg:
> 
>   git grep ' $'
> 
> Fix the new ones here, and the rest in a separate CL.

Acknowledged.

[Jim Stichnoth, 2016-01-13 16:24:55]

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:497: // Sets.
Maybe "unordered_set fields"?

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:526: for (const std::string &Name : ::ExcludedRegisters)
I think it would be more natural and regular if you call
setExcludeRegisters(::ExcludedRegisters) and let setExcludedRegisters iterate
over the collection.

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:557: for (const std::string &Name : ::UseRestrictedRegisters)
same comment as above

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.h#newcod...
src/IceClFlags.h:413: std::unordered_set<std::string> ExcludedRegisters;
I'd prefer IceString instead of std::string.

https://codereview.chromium.org/1571433004/diff/80001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:149: enum RegClass {
FWIW, there is already Ice::RegClass defined in IceOperand.h.

That got me thinking.  There is probably a reasonable way to leverage that
existing register class infrastructure to accomplish this without needing to add
these new data structures.  I'll have to think about that a bit more.

https://codereview.chromium.org/1571433004/diff/80001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:157: Num_RegClasses
Usually this dummy value is named something like RegClass_NUM (to keep the same
prefix on all enum values).

https://codereview.chromium.org/1571433004/diff/80001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:166: static RegClass getRegClass(int32_t RegNum) {
I'm not sure why some of these functions are "static" and others "static
inline".

Probably "class RegARM32" should be changed to "namespace RegARM32", then all of
these would just be "inline".

https://codereview.chromium.org/1571433004/diff/80001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:131: SizeT ClassIndex = RegClassFcn(i);
RegClassFcn is supposed to take an int32_t argument, but i is SizeT.

Maybe types would all work out if i and NumRegs were int32_t?

https://codereview.chromium.org/1571433004/diff/80001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:153: Ostream &Out = Ctx->getStrEmit();
getStrDump()

and usually, the variable is named "Str".
i.e.:
  Ostream &Str = Ctx->getStrDump();

https://codereview.chromium.org/1571433004/diff/80001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/vadd.ll (right):

https://codereview.chromium.org/1571433004/diff/80001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/vadd.ll:21: ; RUN:   -reg-use s20,s22,d20,d22 \
Using a single -reg-use line here but 2 above - any special reason?

[Karl, 2016-01-14 18:02:25]

Description was changed from

==========
Implements include/exclude register lists for ARM translation.

This allows better debugging of register encodings into ARM
instructions, in the integrated assembler.

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

to

==========
Implements include/exclude register lists for translation.

This allows better debugging of register encodings into
instructions, in the integrated assembler.

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

[Karl, 2016-01-14 18:27:19]

This CL now applies command line register filters to all platforms!

https://codereview.chromium.org/1571433004/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/vadd.ll (right):

https://codereview.chromium.org/1571433004/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/vadd.ll:73: ; DIS-NEXT:   14:	ed9daa02
On 2016/01/10 16:51:26, stichnot wrote:
> untabify

Done.

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:497: // Sets.
On 2016/01/13 16:24:54, stichnot wrote:
> Maybe "unordered_set fields"?

Done.

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:526: for (const std::string &Name : ::ExcludedRegisters)
On 2016/01/13 16:24:54, stichnot wrote:
> I think it would be more natural and regular if you call
> setExcludeRegisters(::ExcludedRegisters) and let setExcludedRegisters iterate
> over the collection.

The main reason for not doing this is that we would be exposing cl::list<> to
the API (i.e. users) of this module. Leaving as is.

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:557: for (const std::string &Name : ::UseRestrictedRegisters)
On 2016/01/13 16:24:54, stichnot wrote:
> same comment as above

Same response.

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.h#newcod...
src/IceClFlags.h:413: std::unordered_set<std::string> ExcludedRegisters;
On 2016/01/13 16:24:54, stichnot wrote:
> I'd prefer IceString instead of std::string.

Done.

https://codereview.chromium.org/1571433004/diff/80001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:149: enum RegClass {
On 2016/01/13 16:24:54, stichnot wrote:
> FWIW, there is already Ice::RegClass defined in IceOperand.h.
> 
> That got me thinking.  There is probably a reasonable way to leverage that
> existing register class infrastructure to accomplish this without needing to
add
> these new data structures.  I'll have to think about that a bit more.

Changed to use TypeToRegisterSet vectors for this concept, removing the need for
duplicating register classes.

https://codereview.chromium.org/1571433004/diff/80001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:157: Num_RegClasses
On 2016/01/13 16:24:54, stichnot wrote:
> Usually this dummy value is named something like RegClass_NUM (to keep the
same
> prefix on all enum values).

Done.

https://codereview.chromium.org/1571433004/diff/80001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:166: static RegClass getRegClass(int32_t RegNum) {
On 2016/01/13 16:24:54, stichnot wrote:
> I'm not sure why some of these functions are "static" and others "static
> inline".
> 
> Probably "class RegARM32" should be changed to "namespace RegARM32", then all
of
> these would just be "inline".

The inline's are removed. I left removing class RegARM32 to a separate CL (and
added a TODO).

https://codereview.chromium.org/1571433004/diff/80001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:131: SizeT ClassIndex = RegClassFcn(i);
On 2016/01/13 16:24:54, stichnot wrote:
> RegClassFcn is supposed to take an int32_t argument, but i is SizeT.
> 
> Maybe types would all work out if i and NumRegs were int32_t?

Done.

https://codereview.chromium.org/1571433004/diff/80001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:153: Ostream &Out = Ctx->getStrEmit();
On 2016/01/13 16:24:54, stichnot wrote:
> getStrDump()
> 
> and usually, the variable is named "Str".
> i.e.:
>   Ostream &Str = Ctx->getStrDump();

Done.

https://codereview.chromium.org/1571433004/diff/80001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/vadd.ll (right):

https://codereview.chromium.org/1571433004/diff/80001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/vadd.ll:21: ; RUN:   -reg-use s20,s22,d20,d22 \
On 2016/01/13 16:24:54, stichnot wrote:
> Using a single -reg-use line here but 2 above - any special reason?

Was for testing. Cleaned up for consistency.

[John, 2016-01-14 21:12:49]

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:526: for (const std::string &Name : ::ExcludedRegisters)
On 2016/01/14 18:27:19, Karl wrote:
> On 2016/01/13 16:24:54, stichnot wrote:
> > I think it would be more natural and regular if you call
> > setExcludeRegisters(::ExcludedRegisters) and let setExcludedRegisters
iterate
> > over the collection.
> 
> The main reason for not doing this is that we would be exposing cl::list<> to
> the API (i.e. users) of this module. Leaving as is.

I agree with Jim here. Exposing cl::list<> to the users is not necessarily bad
because the single caller to this routine has access to that type's definition.

If you are **really** bothered by the cl::list<> type, then you could do
something like:

  template <class T>
  void setUseRestrictedRegisters(const T &Value) {
    clearUseRestrictedRegisters();
    UseRestrictedRegisters.insert(Value.begin(), Value.end());
  }

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:557: for (const std::string &Name : ::UseRestrictedRegisters)
On 2016/01/14 18:27:19, Karl wrote:
> On 2016/01/13 16:24:54, stichnot wrote:
> > same comment as above
> 
> Same response.

same suggestion.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:161: const std::unordered_set<std::string> &UseName =
really great candidate for auto:

const auto &UseName

(if nothing else, use IceString instead of std::string)

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:165: for (int32_t RegIndex = 0; RegIndex < NumRegs;
++RegIndex) {
This loop iterates over all registers, but it only ever touches the ones in
UseName. Too bad you need RegIndex below. :(

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:166: IceString RegName(getRegName(RegIndex));
I usually prefer assignments when initializing string objects.

It also seems this object could be marked const.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:179: llvm::SmallBitVector &TypeBitSet =
TypeToRegisterSet[TypeIndex];
can you use a * here?

(yes, I know I am the only one who dislikes non-const references, but they
really help reading the code -- although there's an increase in * chars)

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:190: constexpr const char *LineIndent = "  ";
please use

constexpr char LineIndent = " ";

instead.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:191: SizeT RegSetIndent = 2 * strlen(LineIndent);
and then using llvm::array_lengthof() should let you declare this as a constexpr
too.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.h:305: llvm::SmallBitVector *TypeToRegisterSet,
I know that

llvm::SmallBitVector *

and

llvm::SmallBitVector []

are the same, but when I see the latter I immediately think of an array, where
as with the former, I am tempted to think this is just a ptr.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:296: IceString RegSeparator(", ");
this could be a

const IceString RegSeparator = ", ";

or (maybe, not sure):

constexpr char RegSeparator[] = ", ";

[Karl, 2016-01-14 22:00:53]

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:526: for (const std::string &Name : ::ExcludedRegisters)
On 2016/01/14 21:12:48, John wrote:
> On 2016/01/14 18:27:19, Karl wrote:
> > On 2016/01/13 16:24:54, stichnot wrote:
> > > I think it would be more natural and regular if you call
> > > setExcludeRegisters(::ExcludedRegisters) and let setExcludedRegisters
> iterate
> > > over the collection.
> > 
> > The main reason for not doing this is that we would be exposing cl::list<>
to
> > the API (i.e. users) of this module. Leaving as is.
> 
> I agree with Jim here. Exposing cl::list<> to the users is not necessarily bad
> because the single caller to this routine has access to that type's
definition.
> 
> If you are **really** bothered by the cl::list<> type, then you could do
> something like:
> 
>   template <class T>
>   void setUseRestrictedRegisters(const T &Value) {
>     clearUseRestrictedRegisters();
>     UseRestrictedRegisters.insert(Value.begin(), Value.end());
>   }
> 

I agree that it will work, and the current code would not expose the
command-line processing code, outside of the corresponding cpp file.

However, it is in the header file (as public API). Hence, if someone uses it
down the road, we are exposing the command line processing code. I will leave
this decision to Jim.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:161: const std::unordered_set<std::string> &UseName =
On 2016/01/14 21:12:49, John wrote:
> really great candidate for auto:
> 
> const auto &UseName
> 
> (if nothing else, use IceString instead of std::string)

Did change to IceString.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:165: for (int32_t RegIndex = 0; RegIndex < NumRegs;
++RegIndex) {
On 2016/01/14 21:12:48, John wrote:
> This loop iterates over all registers, but it only ever touches the ones in
> UseName. Too bad you need RegIndex below. :(

Acknowledged.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:166: IceString RegName(getRegName(RegIndex));
On 2016/01/14 21:12:48, John wrote:
> I usually prefer assignments when initializing string objects.
> 
> It also seems this object could be marked const.

I thought the form "clsname foo = bar;" Creates foo with the default
constructor, and then assigns. That is the main reason for not using assignment.
However, it is more readable. Changing...

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:179: llvm::SmallBitVector &TypeBitSet =
TypeToRegisterSet[TypeIndex];
On 2016/01/14 21:12:48, John wrote:
> can you use a * here?
> 
> (yes, I know I am the only one who dislikes non-const references, but they
> really help reading the code -- although there's an increase in * chars)

Done.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:190: constexpr const char *LineIndent = "  ";
On 2016/01/14 21:12:49, John wrote:
> please use
> 
> constexpr char LineIndent = " ";
> 
> instead.

If I remove the const, the type is "char *const" (try it, the compiler will show
it). Hence, I can't remove the "const".

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:191: SizeT RegSetIndent = 2 * strlen(LineIndent);
On 2016/01/14 21:12:48, John wrote:
> and then using llvm::array_lengthof() should let you declare this as a
constexpr
> too.

Able to change using 'constexpr const char LineIndent[] = "  "'.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.h:305: llvm::SmallBitVector *TypeToRegisterSet,
On 2016/01/14 21:12:49, John wrote:
> I know that
> 
> llvm::SmallBitVector *
> 
> and
> 
> llvm::SmallBitVector []
> 
> are the same, but when I see the latter I immediately think of an array, where
> as with the former, I am tempted to think this is just a ptr.
> 

Done.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:296: IceString RegSeparator(", ");
On 2016/01/14 21:12:49, John wrote:
> this could be a
> 
> const IceString RegSeparator = ", ";
> 
> or (maybe, not sure):
> 
> constexpr char RegSeparator[] = ", ";

Used 'constexpr const RegSeparator[] = ", "' and then used
llvm::array_lengthof() to get the size.

[Jim Stichnoth, 2016-01-14 22:04:20]

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:526: for (const std::string &Name : ::ExcludedRegisters)
On 2016/01/14 21:12:48, John wrote:
> On 2016/01/14 18:27:19, Karl wrote:
> > On 2016/01/13 16:24:54, stichnot wrote:
> > > I think it would be more natural and regular if you call
> > > setExcludeRegisters(::ExcludedRegisters) and let setExcludedRegisters
> iterate
> > > over the collection.
> > 
> > The main reason for not doing this is that we would be exposing cl::list<>
to
> > the API (i.e. users) of this module. Leaving as is.
> 
> I agree with Jim here. Exposing cl::list<> to the users is not necessarily bad
> because the single caller to this routine has access to that type's
definition.
> 
> If you are **really** bothered by the cl::list<> type, then you could do
> something like:
> 
>   template <class T>
>   void setUseRestrictedRegisters(const T &Value) {
>     clearUseRestrictedRegisters();
>     UseRestrictedRegisters.insert(Value.begin(), Value.end());
>   }
> 

I ended up retracting my comment after discussion with Karl, because in a
SUPER-MINIMAL build we wouldn't even use llvm::cl (remove all but a few key
command-line arguments such as -O, -o, -threads; and use a simple non-bloated
arg parser), and so I wouldn't like that to leak outside this file.

That said, it turns out that cl::list<std::string> is a subclass of
std::vector<std::string>, so I resurrect my original comment with that in mind.

https://codereview.chromium.org/1571433004/diff/160001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1571433004/diff/160001/src/IceRegistersARM32....
src/IceRegistersARM32.h:52: // TODO(kschimpf) Change this class to a namespace.
I think you should just revert this file entirely, since there are really no
changes directly related to this CL.

I suppose the TODO could stay, or just keep a mental note.

https://codereview.chromium.org/1571433004/diff/160001/src/IceRegistersARM32....
src/IceRegistersARM32.h:191: static IceString getGPRName(SizeT RegNum) {
This would more accurately be named getRegName(), right?

(but maybe left for a separate CL per the comment above?)

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:131: for (SizeT i = 0; i < Bitset.size(); ++i) {
The usual pattern for iterating over the set bits of a bitvector is:

  for (int i = Bitset.find_first(); i != -1; i = Bitset.find_next(i)) { ... }

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:135: indent(Str, LineIndent);
I think you can remove the indent function, and just use:
  Str << std::string(LineIndent, ' ');

Or, just pass in the indent string (instead of its size) as an argument.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:140: Str << ", ";
I would just print a space and drop the comma.  It's more compact, and besides
my OCD triggers when the linebreak omits the comma.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:161: const std::unordered_set<std::string> &UseName =
IceString

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:167: if (UseName.count(RegName)) {
I realize UseName is const, but it would be extra nice if RegName could be
removed from UseName, and then after all processing is finished, any strings
left in UseName be bogus register names.  Those could be printed in an error
message and then pnacl-sz aborts.

Same for ExcludeName.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:189: Ostream &Str = Ctx->getStrEmit();
getStrDump

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:191: SizeT RegSetIndent = 2 * strlen(LineIndent);
Can this be constexpr?  :)
(at least const, surely)

Also, I think size_t is more appropriate here, and for the printRegisterSet
parameter.

https://codereview.chromium.org/1571433004/diff/160001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/vadd.ll (right):

https://codereview.chromium.org/1571433004/diff/160001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/vadd.ll:55: ; IASM-NOT:   vadd
trailing whitespace on this line

[Karl, 2016-01-14 23:54:51]

Hopefully, this CL is getting close to being done.

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1571433004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:526: for (const std::string &Name : ::ExcludedRegisters)
On 2016/01/14 22:04:19, stichnot wrote:
> On 2016/01/14 21:12:48, John wrote:
> > On 2016/01/14 18:27:19, Karl wrote:
> > > On 2016/01/13 16:24:54, stichnot wrote:
> > > > I think it would be more natural and regular if you call
> > > > setExcludeRegisters(::ExcludedRegisters) and let setExcludedRegisters
> > iterate
> > > > over the collection.
> > > 
> > > The main reason for not doing this is that we would be exposing cl::list<>
> to
> > > the API (i.e. users) of this module. Leaving as is.
> > 
> > I agree with Jim here. Exposing cl::list<> to the users is not necessarily
bad
> > because the single caller to this routine has access to that type's
> definition.
> > 
> > If you are **really** bothered by the cl::list<> type, then you could do
> > something like:
> > 
> >   template <class T>
> >   void setUseRestrictedRegisters(const T &Value) {
> >     clearUseRestrictedRegisters();
> >     UseRestrictedRegisters.insert(Value.begin(), Value.end());
> >   }
> > 
> 
> I ended up retracting my comment after discussion with Karl, because in a
> SUPER-MINIMAL build we wouldn't even use llvm::cl (remove all but a few key
> command-line arguments such as -O, -o, -threads; and use a simple non-bloated
> arg parser), and so I wouldn't like that to leak outside this file.
> 
> That said, it turns out that cl::list<std::string> is a subclass of
> std::vector<std::string>, so I resurrect my original comment with that in
mind.

Changed type of register sets to: std::vector<IceString>.

https://codereview.chromium.org/1571433004/diff/160001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1571433004/diff/160001/src/IceRegistersARM32....
src/IceRegistersARM32.h:52: // TODO(kschimpf) Change this class to a namespace.
On 2016/01/14 22:04:19, stichnot wrote:
> I think you should just revert this file entirely, since there are really no
> changes directly related to this CL.
> 
> I suppose the TODO could stay, or just keep a mental note.

Reverted.

https://codereview.chromium.org/1571433004/diff/160001/src/IceRegistersARM32....
src/IceRegistersARM32.h:191: static IceString getGPRName(SizeT RegNum) {
On 2016/01/14 22:04:19, stichnot wrote:
> This would more accurately be named getRegName(), right?
> 
> (but maybe left for a separate CL per the comment above?)

Acknowledged.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:131: for (SizeT i = 0; i < Bitset.size(); ++i) {
On 2016/01/14 22:04:19, stichnot wrote:
> The usual pattern for iterating over the set bits of a bitvector is:
> 
>   for (int i = Bitset.find_first(); i != -1; i = Bitset.find_next(i)) { ... }

Done.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:135: indent(Str, LineIndent);
On 2016/01/14 22:04:19, stichnot wrote:
> I think you can remove the indent function, and just use:
>   Str << std::string(LineIndent, ' ');
> 
> Or, just pass in the indent string (instead of its size) as an argument.

Done.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:140: Str << ", ";
On 2016/01/14 22:04:19, stichnot wrote:
> I would just print a space and drop the comma.  It's more compact, and besides
> my OCD triggers when the linebreak omits the comma.

Done.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:167: if (UseName.count(RegName)) {
On 2016/01/14 22:04:19, stichnot wrote:
> I realize UseName is const, but it would be extra nice if RegName could be
> removed from UseName, and then after all processing is finished, any strings
> left in UseName be bogus register names.  Those could be printed in an error
> message and then pnacl-sz aborts.
> 
> Same for ExcludeName.

Done.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:189: Ostream &Str = Ctx->getStrEmit();
On 2016/01/14 22:04:19, stichnot wrote:
> getStrDump

Done.

https://codereview.chromium.org/1571433004/diff/160001/src/IceTargetLowering....
src/IceTargetLowering.cpp:191: SizeT RegSetIndent = 2 * strlen(LineIndent);
On 2016/01/14 22:04:19, stichnot wrote:
> Can this be constexpr?  :)
> (at least const, surely)
> 
> Also, I think size_t is more appropriate here, and for the printRegisterSet
> parameter.

Done.

https://codereview.chromium.org/1571433004/diff/160001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/vadd.ll (right):

https://codereview.chromium.org/1571433004/diff/160001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/vadd.ll:55: ; IASM-NOT:   vadd
On 2016/01/14 22:04:19, stichnot wrote:
> trailing whitespace on this line

This isn't trailing whitespace! (delete code).

[Jim Stichnoth, 2016-01-15 00:50:56]

I fear we won't reach the centi-comment mark. :)

https://codereview.chromium.org/1571433004/diff/160001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/vadd.ll (right):

https://codereview.chromium.org/1571433004/diff/160001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/vadd.ll:55: ; IASM-NOT:   vadd
On 2016/01/14 23:54:51, Karl wrote:
> On 2016/01/14 22:04:19, stichnot wrote:
> > trailing whitespace on this line
> 
> This isn't trailing whitespace! (delete code).

Not sure what you mean, but there certainly is trailing whitespace:

"; IASM-NOT:   vadd "

If you really mean to match a space character after the vadd (which I believe is
not the intention for the vast majority of existing 'git grep " $"' hits), then
make it look intentional, e.g.:

  ... vadd {{.}}

or even:

  ... vadd{{ }}

https://codereview.chromium.org/1571433004/diff/220001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1571433004/diff/220001/src/IceClFlags.h#newco...
src/IceClFlags.h:120: const std::vector<IceString> &getUseRestrictedRegisters()
const {
Optional save a ton of scary punctuation:

  using StringVector = std::vector<IceString>;

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering....
src/IceTargetLowering.cpp:122: std::function<IceString(int32_t)> getRegName,
My head nearly exploded as I investigated the inconsistency between int32_t and
SizeT in the various getRegName() methods. :(

My conclusion: just keep it int32_t here.

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering....
src/IceTargetLowering.cpp:123: SizeT LineIndent) {
Change SizeT to size_t.

Also, I would change its name to something like LineIndentSpaces to distinguish
it from the differently-typed LineIndent var in the caller.  (Or change the
caller's var name to something like LineIndentString.)

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering....
src/IceTargetLowering.cpp:124: constexpr int RegistersPerLine = 16;
Probably want the types of RegisterPerLine and Count to match.

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering....
src/IceTargetLowering.cpp:126: constexpr int BitsetEnd = -1;
I wouldn't use BitsetEnd, I would just use the raw -1, magic constant
notwithstanding.  All the other code uses -1 (or >= 0 in IceRegAlloc.cpp for
some reason), and the external API explicitly says -1.

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering....
src/IceTargetLowering.cpp:160: llvm::report_fatal_error("Can't find register
use: " + RegName);
It would be nicer if it would tell me *all* the registers I botched, not just
the first one.  One possibility is to initialize an IceString to the empty
string, then append " "+RegName for each offending register, then at the very
end check for a nonempty string.

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:300: Name.replace(Pos,
llvm::array_lengthof(RegSeparator), ":");
I think something is wrong here, because it prints this:
  r0:1,r2:3,r4:5,r6:7
instead of:
  r0:r1,r2:r3,r4:r5,r6:r7

The previous time I checked (patchset 9 I believe), it printed this:
  r0:r1, r2:r3, r4:r5, r6:r7

https://codereview.chromium.org/1571433004/diff/220001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/unknown-arm-reg.ll (right):

https://codereview.chromium.org/1571433004/diff/220001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/unknown-arm-reg.ll:17: ; USE: LLVM ERROR: Can't find
register use: xx9x
If you take my suggestion about reporting *all* botched register names, go ahead
and put two bogus includes and check for that error.

https://codereview.chromium.org/1571433004/diff/220001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/unknown-arm-reg.ll:19: 
Delete trailing whitespace (blank line).

[Karl, 2016-01-15 16:54:25]

https://codereview.chromium.org/1571433004/diff/160001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/vadd.ll (right):

https://codereview.chromium.org/1571433004/diff/160001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/vadd.ll:55: ; IASM-NOT:   vadd
On 2016/01/15 00:50:56, stichnot wrote:
> On 2016/01/14 23:54:51, Karl wrote:
> > On 2016/01/14 22:04:19, stichnot wrote:
> > > trailing whitespace on this line
> > 
> > This isn't trailing whitespace! (delete code).
> 
> Not sure what you mean, but there certainly is trailing whitespace:
> 
> "; IASM-NOT:   vadd "
> 
> If you really mean to match a space character after the vadd (which I believe
is
> not the intention for the vast majority of existing 'git grep " $"' hits),
then
> make it look intentional, e.g.:
> 
>   ... vadd {{.}}
> 
> or even:
> 
>   ... vadd{{ }}

Removed space.

https://codereview.chromium.org/1571433004/diff/220001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1571433004/diff/220001/src/IceClFlags.h#newco...
src/IceClFlags.h:120: const std::vector<IceString> &getUseRestrictedRegisters()
const {
On 2016/01/15 00:50:56, stichnot wrote:
> Optional save a ton of scary punctuation:
> 
>   using StringVector = std::vector<IceString>;

Done.

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering....
src/IceTargetLowering.cpp:122: std::function<IceString(int32_t)> getRegName,
On 2016/01/15 00:50:56, stichnot wrote:
> My head nearly exploded as I investigated the inconsistency between int32_t
and
> SizeT in the various getRegName() methods. :(
> 
> My conclusion: just keep it int32_t here.

Acknowledged.

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering....
src/IceTargetLowering.cpp:123: SizeT LineIndent) {
On 2016/01/15 00:50:56, stichnot wrote:
> Change SizeT to size_t.
> 
> Also, I would change its name to something like LineIndentSpaces to
distinguish
> it from the differently-typed LineIndent var in the caller.  (Or change the
> caller's var name to something like LineIndentString.)

Done.

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering....
src/IceTargetLowering.cpp:124: constexpr int RegistersPerLine = 16;
On 2016/01/15 00:50:56, stichnot wrote:
> Probably want the types of RegisterPerLine and Count to match.

Done.

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering....
src/IceTargetLowering.cpp:126: constexpr int BitsetEnd = -1;
On 2016/01/15 00:50:56, stichnot wrote:
> I wouldn't use BitsetEnd, I would just use the raw -1, magic constant
> notwithstanding.  All the other code uses -1 (or >= 0 in IceRegAlloc.cpp for
> some reason), and the external API explicitly says -1.

Done.

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLowering....
src/IceTargetLowering.cpp:160: llvm::report_fatal_error("Can't find register
use: " + RegName);
On 2016/01/15 00:50:56, stichnot wrote:
> It would be nicer if it would tell me *all* the registers I botched, not just
> the first one.  One possibility is to initialize an IceString to the empty
> string, then append " "+RegName for each offending register, then at the very
> end check for a nonempty string.

Done.

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1571433004/diff/220001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:300: Name.replace(Pos,
llvm::array_lengthof(RegSeparator), ":");
On 2016/01/15 00:50:56, stichnot wrote:
> I think something is wrong here, because it prints this:
>   r0:1,r2:3,r4:5,r6:7
> instead of:
>   r0:r1,r2:r3,r4:r5,r6:r7
> 
> The previous time I checked (patchset 9 I believe), it printed this:
>   r0:r1, r2:r3, r4:r5, r6:r7

Yep. My mistake. I tried to be too intelligent when switching to a constexpr
(from a std::string). Forgot to count the null character.

https://codereview.chromium.org/1571433004/diff/220001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/unknown-arm-reg.ll (right):

https://codereview.chromium.org/1571433004/diff/220001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/unknown-arm-reg.ll:17: ; USE: LLVM ERROR: Can't find
register use: xx9x
On 2016/01/15 00:50:56, stichnot wrote:
> If you take my suggestion about reporting *all* botched register names, go
ahead
> and put two bogus includes and check for that error.

Done.

https://codereview.chromium.org/1571433004/diff/220001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/unknown-arm-reg.ll:19: 
On 2016/01/15 00:50:56, stichnot wrote:
> Delete trailing whitespace (blank line).

Done.

[Jim Stichnoth, 2016-01-15 17:39:13]

otherwise lgtm.

This is an awesome feature that I've wanted for a long time, so thanks for doing
it!

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLowering....
src/IceTargetLowering.cpp:123: IceString LineIndentString) {
const IceString &

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLowering....
src/IceTargetLowering.cpp:156: std::vector<IceString> BadRegNames;
Maybe use StringVector here too?

Note that StringVector is tied to the implementation of cl::list<>, so if LLVM
decided to change cl::list's base class, StringVector would have to change as
well.  That probably wouldn't affect correctness here because only generic STL
container methods are used, but it could potentially pull in bulky code? 
Whatever...

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLowering....
src/IceTargetLowering.cpp:198: const std::string Indent("  ");
const IceString Indent = "  ";

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLowering....
src/IceTargetLowering.cpp:209: Indent + Indent);
Optional: something like:
  const IceString IndentMultiLine = Indent + Indent;
hoisted out of the loop

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1891: for (int i = 0; i < RegARM32::Reg_NUM; ++i)
{
My strong weird preference is to avoid "int" and "unsigned" and instead use
int32_t and uint32_t where possible.  (except when following an external ABI)

[Karl, 2016-01-15 19:07:49]

Description was changed from

==========
Implements include/exclude register lists for translation.

This allows better debugging of register encodings into
instructions, in the integrated assembler.

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

to

==========
Implements include/exclude register lists for translation.

This allows better debugging of register encodings into
instructions, in the integrated assembler.

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, 2016-01-15 19:07:50]

Committed patchset #14 (id:260001) manually as
5403f5dc6b43f077bbbee369bbe178b660654366 (presubmit successful).

[Karl, 2016-01-15 19:08:27]

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLowering....
src/IceTargetLowering.cpp:123: IceString LineIndentString) {
On 2016/01/15 17:39:13, stichnot wrote:
> const IceString &

Done.

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLowering....
src/IceTargetLowering.cpp:156: std::vector<IceString> BadRegNames;
On 2016/01/15 17:39:13, stichnot wrote:
> Maybe use StringVector here too?
> 
> Note that StringVector is tied to the implementation of cl::list<>, so if LLVM
> decided to change cl::list's base class, StringVector would have to change as
> well.  That probably wouldn't affect correctness here because only generic STL
> container methods are used, but it could potentially pull in bulky code? 
> Whatever...

Changed to ClFlags::StringVector.

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLowering....
src/IceTargetLowering.cpp:198: const std::string Indent("  ");
On 2016/01/15 17:39:13, stichnot wrote:
> const IceString Indent = "  ";

Done.

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLowering....
src/IceTargetLowering.cpp:209: Indent + Indent);
On 2016/01/15 17:39:13, stichnot wrote:
> Optional: something like:
>   const IceString IndentMultiLine = Indent + Indent;
> hoisted out of the loop

Done.

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1571433004/diff/240001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1891: for (int i = 0; i < RegARM32::Reg_NUM; ++i)
{
On 2016/01/15 17:39:13, stichnot wrote:
> My strong weird preference is to avoid "int" and "unsigned" and instead use
> int32_t and uint32_t where possible.  (except when following an external ABI)

Done.