Use custom function to format TRIL code #5587

janvrany · 2020-09-30T12:01:54Z

This commit introduces new template function Tril::format() that
is to be used to format TRIL code in tests designed as drop-in
replacement for std::snprintf() except that:

supports limited number of arguments (up to 10, this limit
can/will be removed with allowed use of variadic templates),
support only a (used) subset of *printf-like conversion flags and
the conversion depends on a static type of a value rather than
on specified conversion (different conversion flags are supported
for std::snprintf() compatibility)

Use of a template function and static types rather than *printf-like
conversion flags allows Tril::format() to be used in another templates
and thus will allow to further simplify test creation.

Moreover, this makes TRIL formatting independent on std::snprintf()
which is known to behave differently on different systems. Therefore,
this commit introduces indirection required to fix #5183 and #5308 .

janvrany · 2020-09-30T19:03:55Z

@genie-omr build all

Leonardo2718

Thanks for doing this @janvrany! This will simplify things for sure. A few general comments:

Using fixed width integer types to overload functions (literal() in this case) can sometimes cause ambiguity problems because multiple primitive types can have the same size. For example, both long int and long long int can be 64-bits wide, but are required to be distinct types. int64_t can only be a typedef for one of the two. Because long int (and unsigned long int for that matter) can be implicitly converted to either long long int or unsigned long long int, overloading with int64_t and uint64_t can cause an ambiguity if the latter are typedefs for long long int and unsigned long long int, respectively. See the osx failure [1] and Compiler Explorer example [2].
I think Tril already uses variadic templates for some things (see [3]) so I believe you should be able to use one for Tril::format.
Can you add some tests for this new tool? 🙂

[1] https://ci.eclipse.org/omr/job/PullRequest-osx_x86-64/2021/console

[2] https://godbolt.org/z/x49ffq

[3] https://github.com/eclipse/omr/blob/9d20fb28330885a22ecef2e4b225144d6004b7fd/fvtest/compilertriltest/OpCodeTest.hpp#L81-L82

Leonardo2718 · 2020-09-30T18:43:15Z

fvtest/tril/tril/format.hpp

+
+inline size_t literal(char* dst, const size_t size, const void* value)
+   {
+   return std::snprintf(dst, size, "%" OMR_PRIXPTR, value);


I believe PRIXPTR is technically for intptr_t and clang is reporting a warning about this. Might need to add a reinterpret_cast.

Suggested change

return std::snprintf(dst, size, "%" OMR_PRIXPTR, value);

return std::snprintf(dst, size, "%" OMR_PRIXPTR, reinterpret_cast<intptr_t>(value));

Yes, I've seen that. Will fix as suggested.

Leonardo2718 · 2020-09-30T18:46:31Z

fvtest/tril/tril/format.hpp

+      {
+      if (*fmt == '%')
+         {
+         throw std::runtime_error("found conversion specifier but no value given");


Should there be a special case for %%?

Indeed. Thanks!

fvtest/tril/tril/format.hpp

Leonardo2718 · 2020-10-01T13:00:24Z

fvtest/compilertriltest/CallTest.cpp

-    const auto format_string = "(method return=Int32 args=[Int32] (block (ireturn (icall address=0x%jX args=[Int32] (iload parm=0)) )  ))";
-    std::snprintf(inputTrees, 200, format_string, reinterpret_cast<uintmax_t>(&oracleBoracle));
+    const auto format_string = "(method return=Int32 args=[Int32] (block (ireturn (icall address=0x%X args=[Int32] (iload parm=0)) )  ))";
+    Tril::format(inputTrees, 200, format_string, reinterpret_cast<void*>(&oracleBoracle));


Maybe this can become a static_cast now?

Suggested change

Tril::format(inputTrees, 200, format_string, reinterpret_cast<void*>(&oracleBoracle));

Tril::format(inputTrees, 200, format_string, static_cast<void*>(&oracleBoracle));

janvrany · 2020-10-01T18:47:49Z

Thanks for this review, @Leonardo2718 - and for all that will follow as this would - I'm afraid - take a couple of iterations!

Can you add some tests for this new tool?

Yeah, I guess so. I was afraid somebody would ask for them. :-)

I think Tril already uses variadic templates for some things

Brilliant, I was put off by explicit mentioning in Unsupported C++11 Features

Just to double check, is the syntax for conversion specifiers being kept just to maintain compatibility with the existing tests? Given that the correct conversion specifier is derived from the type T and that the explicit specifiers are ignored,...

I'm actually thinking of using them. The idea is to have %s that would actually derive "best-bet" specifier from T (as it is now) and for everything else use them to make sure they're "correct". something like (after skipping modifiers like ll or j):

if (*fmt == 'd') {
    assert( std::is_integral<T>::value && "%d used but value is of an integral type" );
    std::snprintf(buf, size, "%" PRId64, static_cast<int64_t>(value)); 
} else if (...)

This may provide more flexbility (removes "one formatting rules them all" for any given type), may provide more
"type safety" (if desired) and also should address

Using fixed width integer types to overload functions (literal() in this case) can sometimes cause ambiguity problems
I believe - I have to try.

I'll make a second pass next week. Thanks for having a look!

Leonardo2718 · 2020-10-02T13:58:23Z

[...] and for all that will follow as this would - I'm afraid - take a couple of iterations!

Yeah..... sorry about that 😅

Brilliant, I was put off by explicit mentioning in Unsupported C++11 Features

Hm, we've upgraded some of our compilers since the last time that document was revised. I've opened #5592 to revisit that list and bring it up to date.

I'm actually thinking of using them. The idea is to have %s that would actually derive "best-bet" specifier from T (as it is now) and for everything else use them to make sure they're "correct". something like (after skipping modifiers like ll or j):
   if (*fmt == 'd') {
       assert( std::is_integral<T>::value && "%d used but value is of an integral type" );
       std::snprintf(buf, size, "%" PRId64, static_cast<int64_t>(value)); 
   } else if (...)
This may provide more flexbility (removes "one formatting rules them all" for any given type), may provide more
"type safety" (if desired) and also should address

Oh, interesting! 👍

janvrany · 2020-10-12T11:34:26Z

@genie-omr build all

0xdaryl · 2021-03-05T00:53:27Z

@genie-omr build all

Re-running CI to see if there are any failures.

janvrany · 2021-03-05T09:31:31Z

Just to let you know I still plan to finish this, but got distracted by all the J9, CI, linkage...will get back to it.

0xdaryl · 2021-03-05T12:34:36Z

Just to let you know I still plan to finish this, but got distracted by all the J9, CI, linkage...will get back to it.

Cool. Thanks @janvrany.

janvrany · 2023-12-12T20:04:15Z

jenkins build all

janvrany · 2023-12-12T20:15:14Z

jenkins build all

janvrany · 2023-12-12T22:17:59Z

jenkins build zos aix

janvrany · 2023-12-13T15:00:17Z

jenkins build zos aix

janvrany · 2023-12-13T16:16:54Z

jenkins build zos

janvrany · 2023-12-15T12:20:03Z

jenkins build zos aix

This commit introduces new template function `Tril::format()` that is to be used to format TRIL code in tests designed as drop-in replacement for `std::snprintf()` except that: 1. support only a (used) subset of *printf-like conversion specifiers, 2. "modifiers" like l, ll, h, hh, j, I, I32 and I64 are ignored and 3. used conversion specifier and a static type of actual parameter must match (it is not possible to format `double` using `%X` conversion) Use of a template function and static types rather than *printf-like conversion flags allows `Tril::format()` to be used in another templates and thus will allow to further simplify test creation. Moreover, this makes TRIL formatting independent on `std::snprintf()` which is known to behave differently on different systems. Therefore, this commit introduces indirection required to fix eclipse-omr#5183 and eclipse-omr#5324.

Since XLC compiler used at a time on AIX and ZOS does not support std::make_signed, this commit provides simplified drop-in replacement for use in reinterpret_as_signed(). Once XLC compiler on AIX and ZOS is upgraded to support std::make_signed, this simplified version can be removed in favour of standard version.

…printf()

…tf()

…ntf()

…std::snprintf()

…ntf()

…d::snprintf()

…tf()

…::snprintf()

…f std::snprintf()

…::snprintf()

…tf()

janvrany · 2023-12-15T18:06:47Z

jenkins build all

janvrany · 2023-12-15T19:37:04Z

I finally got back to it hoping that over time compilers would be bumped to newer versions which would solve some mysterious template-related error messages I've got couple years ago.

After a lot of trial and error on platforms I have no access to I thing I've got it working. The failure on macOS seems unrelated (jitbuilder API generator test), the failure on RISC-V seems to be #6905. On my RISC-V systems, comptest passes.

FYI @0xdaryl as I'm not sure who maintains TRIL these days.

janvrany requested review from 0xdaryl, fjeremic and Leonardo2718 as code owners September 30, 2020 12:01

github-actions bot added comp:test comp:tril labels Sep 30, 2020

Leonardo2718 self-assigned this Sep 30, 2020

Leonardo2718 suggested changes Oct 1, 2020

View reviewed changes

janvrany force-pushed the pr/use-custom-tril-format branch 3 times, most recently from 21f8a89 to 5370ed3 Compare October 12, 2020 10:17

janvrany force-pushed the pr/use-custom-tril-format branch from 5370ed3 to 00248d1 Compare December 12, 2023 20:02

janvrany force-pushed the pr/use-custom-tril-format branch from 00248d1 to d7e9c57 Compare December 12, 2023 20:14

janvrany force-pushed the pr/use-custom-tril-format branch from d7e9c57 to 27f372d Compare December 12, 2023 22:17

janvrany force-pushed the pr/use-custom-tril-format branch 5 times, most recently from 0e8a93b to 7f9b99c Compare December 14, 2023 18:54

janvrany force-pushed the pr/use-custom-tril-format branch from 7b277b5 to 55b7637 Compare December 15, 2023 11:49

janvrany added 22 commits December 15, 2023 17:41

Like previous commit, but for std::is_same

f14bdc2

Like previous commit, but for std::is_integral

671ba7a

Like previous commit, but for std::is_unsigned

9d2ff16

Like previous commit, but for std::is_pointer

8671282

Like previous commit, but for std::is_floating_point

e8cfd53

Convert ArithmeticTest tests to use Tril::format() instead of std::sn…

4db9382

…printf()

Convert ArrayTest tests to use Tril::format() instead of std::snprintf()

995ea89

Convert BitPermute tests to use Tril::format() instead of std::snprin…

22072f1

…tf()

Convert CallTest tests to use Tril::format() instead of std::snprintf()

3308657

Convert CompareTest tests to use Tril::format() instead of std::snpri…

204b896

…ntf()

Convert IfxcmpgeReductionTest tests to use Tril::format() instead of …

e9251af

…std::snprintf()

Convert LinkageTest tests to use Tril::format() instead of std::snpri…

f7503d7

…ntf()

Convert LogicalTest tests to use Tril::format() instead of std::snpri…

9b01afe

…ntf()

Convert LongAndAsRotateTest tests to use Tril::format() instead of st…

de4668f

…d::snprintf()

Convert MaxMinTest tests to use Tril::format() instead of std::snprin…

18e04eb

…tf()

Convert SelectTest tests to use Tril::format() instead of std::snprin…

0760347

…tf()

Convert ShiftAndRotateTest tests to use Tril::format() instead of std…

505dbdd

…::snprintf()

Convert SimpifierFoldAbsNegTest tests to use Tril::format() instead o…

13024c5

…f std::snprintf()

Convert TypeConversionTest tests to use Tril::format() instead of std…

47c882c

…::snprintf()

Convert VectorTest tests to use Tril::format() instead of std::snprin…

ad7275b

…tf()

janvrany force-pushed the pr/use-custom-tril-format branch from 187c1bd to ad7275b Compare December 15, 2023 17:56

janvrany mentioned this pull request Feb 9, 2024

TRIL tests on z/OS generates non-standard NaN formatting #7254

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use custom function to format TRIL code #5587

Use custom function to format TRIL code #5587

janvrany commented Sep 30, 2020

janvrany commented Sep 30, 2020

Leonardo2718 left a comment

Leonardo2718 Sep 30, 2020

janvrany Oct 1, 2020

Leonardo2718 Sep 30, 2020

janvrany Oct 1, 2020

Leonardo2718 Oct 1, 2020

janvrany commented Oct 1, 2020

Leonardo2718 commented Oct 2, 2020

janvrany commented Oct 12, 2020

0xdaryl commented Mar 5, 2021

janvrany commented Mar 5, 2021

0xdaryl commented Mar 5, 2021

janvrany commented Dec 12, 2023

janvrany commented Dec 12, 2023

janvrany commented Dec 12, 2023

janvrany commented Dec 13, 2023

janvrany commented Dec 13, 2023

janvrany commented Dec 15, 2023

janvrany commented Dec 15, 2023

janvrany commented Dec 15, 2023

	return std::snprintf(dst, size, "%" OMR_PRIXPTR, value);
	return std::snprintf(dst, size, "%" OMR_PRIXPTR, reinterpret_cast<intptr_t>(value));

	Tril::format(inputTrees, 200, format_string, reinterpret_cast<void*>(&oracleBoracle));
	Tril::format(inputTrees, 200, format_string, static_cast<void*>(&oracleBoracle));

Use custom function to format TRIL code #5587

Are you sure you want to change the base?

Use custom function to format TRIL code #5587

Conversation

janvrany commented Sep 30, 2020

janvrany commented Sep 30, 2020

Leonardo2718 left a comment

Choose a reason for hiding this comment

Leonardo2718 Sep 30, 2020

Choose a reason for hiding this comment

janvrany Oct 1, 2020

Choose a reason for hiding this comment

Leonardo2718 Sep 30, 2020

Choose a reason for hiding this comment

janvrany Oct 1, 2020

Choose a reason for hiding this comment

Leonardo2718 Oct 1, 2020

Choose a reason for hiding this comment

janvrany commented Oct 1, 2020

Leonardo2718 commented Oct 2, 2020

janvrany commented Oct 12, 2020

0xdaryl commented Mar 5, 2021

janvrany commented Mar 5, 2021

0xdaryl commented Mar 5, 2021

janvrany commented Dec 12, 2023

janvrany commented Dec 12, 2023

janvrany commented Dec 12, 2023

janvrany commented Dec 13, 2023

janvrany commented Dec 13, 2023

janvrany commented Dec 15, 2023

janvrany commented Dec 15, 2023

janvrany commented Dec 15, 2023