Remove call to PNaClABIProps::isAllowedAlignment from subzero.

Remove call to PNaClABIProps::isAllowedAlignment from subzero.

Also removes the need for DataLayout.

BUG=None
R=stichnot@chromium.org

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

[Karl, 2014-12-08 22:48:01]

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

[Jim Stichnoth, 2014-12-08 23:14:47]

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1438: bool isValidLoadStoreAlignment(size_t Alignment,
Ice::Type Ty,
If you change the type of Alignment, should you also propagate the change back
to the callers, etc.?

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1442: if (isAllowedAlignment(Alignment, Ty)) return
true;
return on separate line (i.e., make format-diff)

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1454: bool isAllowedAlignment(size_t Alignment,
Ice::Type Ty) {
Can this be const?

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1455: return Alignment == typeAlignInBytes(Ty) ||
This implementation doesn't seem to match the original implementation, in
particular PNaClABIProps contains the comment:

  // Non-atomic integer operations must always use "align 1", since we do not
  // want the backend to generate code with non-portable undefined behaviour
  // (such as misaligned access faults) if user code specifies "align 4" but
  // uses a misaligned pointer.

Which one is correct?

[Karl, 2014-12-09 21:20:45]

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1438: bool isValidLoadStoreAlignment(size_t Alignment,
Ice::Type Ty,
On 2014/12/08 23:14:47, stichnot wrote:
> If you change the type of Alignment, should you also propagate the change back
> to the callers, etc.?

Done.

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1442: if (isAllowedAlignment(Alignment, Ty)) return
true;
On 2014/12/08 23:14:47, stichnot wrote:
> return on separate line (i.e., make format-diff)

Done.

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1454: bool isAllowedAlignment(size_t Alignment,
Ice::Type Ty) {
On 2014/12/08 23:14:47, stichnot wrote:
> Can this be const?

Done.

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1455: return Alignment == typeAlignInBytes(Ty) ||
On 2014/12/08 23:14:47, stichnot wrote:
> This implementation doesn't seem to match the original implementation, in
> particular PNaClABIProps contains the comment:
> 
>   // Non-atomic integer operations must always use "align 1", since we do not
>   // want the backend to generate code with non-portable undefined behaviour
>   // (such as misaligned access faults) if user code specifies "align 4" but
>   // uses a misaligned pointer.
> 
> Which one is correct?

This code doesn't match the comment (which is out of date), but it does match
the implementation. Further, it matches the PNaClABI documentation.

[jvoung (off chromium), 2014-12-09 21:32:10]

https://codereview.chromium.org/789483003/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/789483003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1456: return Alignment == typeAlignInBytes(Ty) ||
I thought it was more like,

if(isScalarInteger(Ty))
  return Alignment == 1
else
  return Alignment == 1 || Alignment == typeAlignInBytes(ElementType(Ty))

Otherwise, this seems to allow integer i32 scalars to have a load alignment of
4?

[jvoung (off chromium), 2014-12-09 21:34:03]

https://codereview.chromium.org/789483003/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/789483003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1456: return Alignment == typeAlignInBytes(Ty) ||
On 2014/12/09 21:32:09, jvoung wrote:
> I thought it was more like,
> 
> if(isScalarInteger(Ty))
>   return Alignment == 1
> else
>   return Alignment == 1 || Alignment == typeAlignInBytes(ElementType(Ty))
> 
> Otherwise, this seems to allow integer i32 scalars to have a load alignment of
> 4?

Ah n/m I didn't look at what typeAlignInBytes() did.

[Jim Stichnoth, 2014-12-09 21:48:24]

lgtm

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/789483003/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1455: return Alignment == typeAlignInBytes(Ty) ||
On 2014/12/09 21:20:45, Karl wrote:
> On 2014/12/08 23:14:47, stichnot wrote:
> > This implementation doesn't seem to match the original implementation, in
> > particular PNaClABIProps contains the comment:
> > 
> >   // Non-atomic integer operations must always use "align 1", since we do
not
> >   // want the backend to generate code with non-portable undefined behaviour
> >   // (such as misaligned access faults) if user code specifies "align 4" but
> >   // uses a misaligned pointer.
> > 
> > Which one is correct?
> 
> This code doesn't match the comment (which is out of date), but it does match
> the implementation. Further, it matches the PNaClABI documentation.

Thanks, my world-view is restored. :)

[Karl, 2014-12-09 21:50:16]

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