This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ]:[z/OS]:[libcxx]: fix the mask in stage2_float_loop function
ClosedPublic

Authored by NancyWang2222 on Feb 3 2022, 11:23 AM.

Details

Summary

This patch is to fix issue related to __stage2_float_loop function, float point value comparison is not working on EBCDIC mode because the mask is hard-coded and assumes character is ASCII, fix is to use toupper function when do the comparison.

Diff Detail

Event Timeline

NancyWang2222 requested review of this revision.Feb 3 2022, 11:23 AM
NancyWang2222 created this revision.

remove an extra curly bracket

LGTM, but I think you need a libc++ group reviewer approval

This revision is now accepted and ready to land.Feb 3 2022, 12:55 PM
ldionne added a subscriber: ldionne.Feb 7 2022, 2:13 PM
ldionne added inline comments.
libcxx/include/locale
549

This is going to be really naive, but can you explain why exp & 0x7F this is even equivalent to toupper(exp)?

I understand the equivalence for c & 0x5f based on the table below, but I don't see the link for 0x7F:

0x41  01000001    A
0x61  01100001    a
0x42  01000010    B
0x62  01100010    b
0x43  01000011    C
0x63  01100011    c
0x44  01000100    D
0x64  01100100    d
0x45  01000101    E
0x65  01100101    e
0x46  01000110    F
0x66  01100110    f
0x47  01000111    G
0x67  01100111    g
0x48  01001000    H
0x68  01101000    h
0x49  01001001    I
0x69  01101001    i
[...]
0x58  01011000    X
0x78  01111000    x
0x59  01011001    Y
0x79  01111001    y
0x5A  01011010    Z
0x7A  01111010    z

0x5F  01011111
0x7F  01111111
0x80  10000000
560

I also don't understand this one.

SeanP added inline comments.Feb 7 2022, 3:41 PM
libcxx/include/locale
549

The 0x7f is tied to the "|0x80" below. It is an ugly hack that takes advantage of the fact all ascii characters are in the range of 0-127. The hack enables or disables the check on line 558 ((__x&0x5f)) == __exp) from matching the letter 'E' as a valid character in the mantissa or the start of the exponent. When the high order bit is set in __exp, the test on line 558 will match 'a' and 'A', but never 'e' or 'E', but the test here will match 'e' and 'E'.

This hack doesn't work for ebcdic since the characters are in the range of 0-255. To make the code work with both we changed the code to make __exp 'e' instead of 'E' plus the high order bit (line 560) and then on this line we upper case both __exp and the character to get the case insensitive compare and just upper case the character on line 558 to get the context sensitive compare.

I recall the logic is:

  • line 549 - match any 'e'/'E' in the string
  • line 558 - only match the first 'e'/'E'

Making __exp lower case instead of setting the high order bit achieves the same results.

if no more comments on this PR. plan to land it . Thanks for the feedback.

NancyWang2222 marked an inline comment as done.Feb 14 2022, 8:48 AM
NancyWang2222 set the repository for this revision to rG LLVM Github Monorepo.Feb 14 2022, 8:57 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 14 2022, 8:57 AM
This revision now requires review to proceed.Feb 14 2022, 8:57 AM

kindly ping for review or approval from libcxx group :)

SeanP added a comment.Feb 16 2022, 9:52 AM

@ldionne, did the explanation help? I was thinking we could clean this logic up even better by adding a reference parameter to __stage2_float_loop() to track whether __exp has been seen or not rather than trying to store state in the character.

The logic around line 558 would become:

else if (!__seen_exp && _VSTD::toupper(__x) == __exp)
{
    __seen_exp = true;

And the compare on line 549 would simplify to:

if (__a_end == __a || (_VSTD::toupper(__a_end[-1]) == __exp))

Can I have 2nd review from libcxx. Thanks.

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

libcxx/include/locale
549

Same below.

NancyWang2222 added a comment.EditedFeb 18 2022, 1:57 PM

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

Thanks for providing information, can you provide more detail about what change need for libcxx/test/localization, the patch will fix bunch of Assert d1==d2 issues in std/numerics/rand/rand.dis/rand.dist.bern/ on z/OS related to float point value.

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

Thanks for providing information, can you provide more detail about what change need for libcxx/test/localization, the patch will fix bunch of Assert d1==d2 issues in std/numerics/rand/rand.dis/rand.dist.bern/ on z/OS related to float point value.

I would have expected test failures in locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, since this patch changes behavior of std::num_get on AIX IIUC. So either I'm not understanding something here or there are some test cases missing. Wait there are also no // XFAIL: AIX in std/numerics/rand.dis/rand.dis.bern/. Are these tests currently disabled on AIX somehow?

NancyWang2222 added a subscriber: daltenty.EditedFeb 18 2022, 3:27 PM

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

Thanks for providing information, can you provide more detail about what change need for libcxx/test/localization, the patch will fix bunch of Assert d1==d2 issues in std/numerics/rand/rand.dis/rand.dist.bern/ on z/OS related to float point value.

I would have expected test failures in locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, since this patch changes behavior of std::num_get on AIX IIUC. So either I'm not understanding something here or there are some test cases missing. Wait there are also no // XFAIL: AIX in std/numerics/rand.dis/rand.dis.bern/. Are these tests currently disabled on AIX somehow?

I will take a look at locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, I didn't see this test case failing on z/OS. the patch will fix the following test cases on z/OS because z/OS has EBCDIC mode, I wasn't sure about AIX. @daltenty @hubert.reinterpretcast may know.

libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bernoulli/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.geo/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.negbin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.cauchy/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.chisq/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.f/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.lognormal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.normal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.t/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.exp/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.extreme/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.gamma/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.poisson/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.weibull/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.real/io.pass.cpp

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

Thanks for providing information, can you provide more detail about what change need for libcxx/test/localization, the patch will fix bunch of Assert d1==d2 issues in std/numerics/rand/rand.dis/rand.dist.bern/ on z/OS related to float point value.

I would have expected test failures in locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, since this patch changes behavior of std::num_get on AIX IIUC. So either I'm not understanding something here or there are some test cases missing. Wait there are also no // XFAIL: AIX in std/numerics/rand.dis/rand.dis.bern/. Are these tests currently disabled on AIX somehow?

I will take a look at locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, I didn't see this test case failing on z/OS. the patch will fix the following test cases on z/OS because z/OS has EBCDIC mode, I wasn't sure about AIX. @daltenty @hubert.reinterpretcast may know.

libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bernoulli/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.geo/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.negbin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.cauchy/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.chisq/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.f/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.lognormal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.normal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.t/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.exp/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.extreme/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.gamma/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.poisson/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.weibull/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.real/io.pass.cpp

Oh, sorry. For some reason I thought z/OS and AIX were the same thing. Are you working on libc++ officially supporting z/OS? If that is the case, maybe it would be a good idea to add a CI node for that?

NancyWang2222 added a comment.EditedFeb 18 2022, 3:48 PM

Oh, sorry. For some reason I thought z/OS and AIX were the same thing. Are you working on libc++ officially supporting z/OS? If that is the case, maybe it would be a good idea to add a CI node for that?

no problem. yeah. we are working on making libc++ supporting z/OS as well. good idea. we will consider to add CI node for z/OS in future.

SeanP added a comment.EditedFeb 22 2022, 7:28 AM

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

Thanks for providing information, can you provide more detail about what change need for libcxx/test/localization, the patch will fix bunch of Assert d1==d2 issues in std/numerics/rand/rand.dis/rand.dist.bern/ on z/OS related to float point value.

I would have expected test failures in locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, since this patch changes behavior of std::num_get on AIX IIUC. So either I'm not understanding something here or there are some test cases missing. Wait there are also no // XFAIL: AIX in std/numerics/rand.dis/rand.dis.bern/. Are these tests currently disabled on AIX somehow?

I will take a look at locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, I didn't see this test case failing on z/OS. the patch will fix the following test cases on z/OS because z/OS has EBCDIC mode, I wasn't sure about AIX. @daltenty @hubert.reinterpretcast may know.

libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bernoulli/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.geo/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.negbin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.cauchy/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.chisq/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.f/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.lognormal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.normal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.t/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.exp/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.extreme/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.gamma/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.poisson/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.weibull/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.real/io.pass.cpp

Oh, sorry. For some reason I thought z/OS and AIX were the same thing. Are you working on libc++ officially supporting z/OS? If that is the case, maybe it would be a good idea to add a CI node for that?

The exp parameter that is being updated is internal to the floating point. The changes we are making are consistent with the way the parameter was being updated. That is why it doesn't impact anything else.

I agree having a CI node for z/OS would be good. Hopefully we'll have one pretty soon.

ldionne accepted this revision.Mar 2 2022, 8:28 AM

Thanks for the explanation.

This revision is now accepted and ready to land.Mar 2 2022, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 8:28 AM

Please use std:: instead of _VSTD::, though (as suggested by @philnik)

NancyWang2222 added a comment.EditedOct 4 2022, 2:17 PM

Please use std:: instead of _VSTD::, though (as suggested by @philnik)

hi Louis, sorry for late reply, haven't been active working on this for a while. I noticed there are many other places using '_VSTD::' in 'libcxx/include/locale' as well. I think should replace all of them to "std::" in separate patch. Thanks

libcxx/include/locale
549

This needs to use macro _VSTD which is defined as std, same with other line.

Please use std:: instead of _VSTD::, though (as suggested by @philnik)

hi Louis, sorry for late reply, haven't been active working on this for a while. I noticed there are many other places using '_VSTD::' in 'libcxx/include/locale' as well. I think should replace all of them to "std::" in separate patch. Thanks

We don't want to replace all the _VSTDs in libc++ at once, since there we still have it in a lot of places (> 3300 uses), but we don't want to introduce new uses. The idea is to remove _VSTD when we touch the code anyways.

use std:: to replace _VSTD::

NancyWang2222 marked 3 inline comments as done.Oct 14 2022, 7:03 AM