Page MenuHomePhabricator

rodgert (Thomas Rodgers)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 20 2018, 10:20 AM (21 w, 5 d)

Recent Activity

Tue, Apr 23

rodgert accepted D59792: [pstl] Make the default backend be the serial backend and always provide parallel policies.

I am good with this change

Tue, Apr 23, 9:35 AM · Restricted Project, Restricted Project

Apr 16 2019

rodgert added a reviewer for D60595: [pstl] Clean up parameter uglifications: ldionne.
Apr 16 2019, 10:12 AM · Restricted Project

Apr 12 2019

rodgert added a comment to D60464: [pstl] Setup the _PSTL_VERSION macro like _LIBCPP_VERSION, and add release notes.

It's implicit in C++. I mean we can decide to do it like the rest of libc++, but we don't have to. For libc++, the reasoning is that it's needed to test in a freestanding environment.

@rodgert @MikeDvorskiy Do you guys want to consistently use return 0; at the end of main? If so, I'll fix this (and any other test that doesn't do it).

Apr 12 2019, 11:07 AM · Restricted Project, Restricted Project

Apr 11 2019

rodgert created D60595: [pstl] Clean up parameter uglifications.
Apr 11 2019, 5:56 PM · Restricted Project
rodgert accepted D60535: [pstl] Remove the stdlib headers from the PSTL and move them to the tests.

I'm not opposed to this change, but I wonder, should we then just remove the extra 'internal' directory, so that we get -

Apr 11 2019, 2:14 PM · Restricted Project, Restricted Project

Apr 10 2019

rodgert accepted D60521: [pstl] Move to single underscore-capital for macros and include guards.
Apr 10 2019, 12:45 PM · Restricted Project
rodgert added reviewers for D60525: [pstl] Fix header inclusion order failures: ldionne, MikeDvorskiy.
Apr 10 2019, 11:33 AM · Restricted Project, Restricted Project
rodgert created D60525: [pstl] Fix header inclusion order failures.
Apr 10 2019, 11:33 AM · Restricted Project, Restricted Project
rodgert added a comment to D60480: [WIP] integration of pstl into libc++.
In D60480#1461555, @jfb wrote:

Not sure what is up with phabricator, but it won't let me respond inline to EricWF, specifically -

__FIRST_MOVER_ADVANTAGE

libc++ and libstdc++ have different 'uglification' protocols. As I said to mclow early on in this process, If I'm the one that has to do the grunt work of *all* of the uglification (very much has been case, BTW), it's going to follow libstdc++ convention as much as is possible.

libc++ code should follow libc++ conventions.

Just to be clear: PSTL is neither libc++ nor libstdc++. Louis kinda said so above, and Tom is making that point too, but I want to reiterate it. It's overall a silly bikeshedding point, but it's critical to keep everyone happy. PSTL isn't more "ours" than "theirs" (or rather, it's "ours" in that we're all in the same C++ team). This point was critical in getting PSTL to be a shared resource in the first place, and I don't want to walk back this commitment.

As Tom mentions above, we've talked about having tooling to massage PSTL to libc++'s and libstdc++'s liking on integration. Without volunteers to write said tooling, we should just accept slight ache when integrating PSTL, as we see here when integrating it into libc++.

The process of applying this kind of change is very error prone. I still have a patch coming that fixes a few cases I missed. That's the bigger argument IMO for tooling than the specific format.

I think I'm missing yours and JF's point. I don't understand why you need any tooling. All I'm saying is that PSTL being under the LLVM umbrella should conform to the usual LLVM code style, which used single underscore + capitals for macros unless I'm mistaken. libstdc++ and libc++ wouldn't apply any code transformation on the PSTL sources after that, it would just use it as a separate library and there wouldn't be very many PSTL macros used inside libstdc++/libc++. Only those macros that are part of the PSTL's interface, and yes, those would be using the PSTL's naming convention.

IOW, this is something that we simply missed in the initial check-in.

Apr 10 2019, 10:37 AM · Restricted Project
rodgert updated the diff for D60249: [pstl] Replace direct use of assert() with __PSTL_ASSERT.
  • [pstl] Supply definition of __PSTL_ASSERT() only if not already defined
Apr 10 2019, 10:25 AM · Restricted Project
rodgert added a comment to D60480: [WIP] integration of pstl into libc++.
In D60480#1461555, @jfb wrote:

Not sure what is up with phabricator, but it won't let me respond inline to EricWF, specifically -

__FIRST_MOVER_ADVANTAGE

libc++ and libstdc++ have different 'uglification' protocols. As I said to mclow early on in this process, If I'm the one that has to do the grunt work of *all* of the uglification (very much has been case, BTW), it's going to follow libstdc++ convention as much as is possible.

libc++ code should follow libc++ conventions.

Just to be clear: PSTL is neither libc++ nor libstdc++. Louis kinda said so above, and Tom is making that point too, but I want to reiterate it. It's overall a silly bikeshedding point, but it's critical to keep everyone happy. PSTL isn't more "ours" than "theirs" (or rather, it's "ours" in that we're all in the same C++ team). This point was critical in getting PSTL to be a shared resource in the first place, and I don't want to walk back this commitment.

As Tom mentions above, we've talked about having tooling to massage PSTL to libc++'s and libstdc++'s liking on integration. Without volunteers to write said tooling, we should just accept slight ache when integrating PSTL, as we see here when integrating it into libc++.

Apr 10 2019, 10:12 AM · Restricted Project
rodgert added a comment to D60480: [WIP] integration of pstl into libc++.

I also suggest:

  • Removing the <algorithm> & friends headers from PSTL itself. Those don't belong there, they belong with the standard library.

    WDYT @rodgert ?

If we remove them, then we sort of lose the ability to test pstl outside of standard library, no? I'm not going to be testing or packaging libc++ most likely, but I am going to be contributing changes here and running tests within the context of libstdc++. Having the ability to test changes to the library independent of any integration with another standard library is still useful for me.

I believe those headers should be provided by a dummy standard library that we use only to run the tests. Basically we'd have a dummy standard library in pstl/test that would define these headers, and that's what we would run the tests against.

That would work for me...AND...we can probably remove the per-test #ifdef for the standalone checks, centralize it in the dummy standard headers in 'pstl/test' which would be much cleaner than what is currently there.

Ok, I'll submit a patch for that.

Lastly, about the __PSTL_XXX vs _PSTL_XXX: This patch is using what's in PSTL today, that's all. As a separate change, I thought about changing all __PSTL_XXX macros to be _PSTL_XXX to follow the more common convention (underscore-capital or double-underscore-lowercase), but this is a change separate from the integration into libc++.

JF and I discussed possibly some tooling for this sort of thing. I'm still interested in looking at that, I just don't see a lot of value in changing it by hand just to comply with conflicting conventions between standard library implementations.

I don't think the conventions used in PSTL should conform to the coding standard in any one standard library implementation. It's a library on its own. I think it should conform to its own coding standards, and in this case, I think those coding standard should be that of LLVM. Does this point of view make sense?

Apr 10 2019, 10:10 AM · Restricted Project
rodgert added a comment to D60480: [WIP] integration of pstl into libc++.

The purpose of this review is to expose how I plan to integrate the parallel STL into libc++, and to discuss some changes that are desirable in pstl for this to be possible. This is a work in progress, obviously, and it's meant as a concrete support for having discussions.

Concrete changes I think we need to make in pstl:

  • Rename all the files to double underscores.

This isn't a libstdc++ convention, but I'm not opposed.

The problem with not doing that is that our headers could collide with user's header files.

Apr 10 2019, 8:33 AM · Restricted Project

Apr 9 2019

rodgert added a comment to D60480: [WIP] integration of pstl into libc++.

Not sure what is up with phabricator, but it won't let me respond inline to EricWF, specifically -

Apr 9 2019, 10:28 PM · Restricted Project
rodgert added a comment to D60480: [WIP] integration of pstl into libc++.

The purpose of this review is to expose how I plan to integrate the parallel STL into libc++, and to discuss some changes that are desirable in pstl for this to be possible. This is a work in progress, obviously, and it's meant as a concrete support for having discussions.

Concrete changes I think we need to make in pstl:

  • Rename all the files to double underscores.

This isn't a libstdc++ convention, but I'm not opposed.

Apr 9 2019, 10:17 PM · Restricted Project

Apr 5 2019

rodgert added inline comments to D60249: [pstl] Replace direct use of assert() with __PSTL_ASSERT.
Apr 5 2019, 1:28 PM · Restricted Project
rodgert added a reviewer for D60249: [pstl] Replace direct use of assert() with __PSTL_ASSERT: ldionne.
Apr 5 2019, 1:16 PM · Restricted Project

Apr 4 2019

rodgert added a comment to D59813: The other fix for equal algo.

Of course. I will configure "ninja" for running the upstream test.

Apr 4 2019, 12:36 PM

Apr 3 2019

rodgert created D60249: [pstl] Replace direct use of assert() with __PSTL_ASSERT.
Apr 3 2019, 10:39 PM · Restricted Project

Mar 29 2019

rodgert accepted D59767: [pstl] Indent preprocessor directives as part of the clang-format rules.

I am also in favor of this.

Mar 29 2019, 1:25 PM · Restricted Project
rodgert created D60009: [pstl] Add additional namespace qualifications.
Mar 29 2019, 12:56 PM · Restricted Project

Mar 28 2019

rodgert abandoned D59199: Uglify unseq_backend_simd.

Committed by other, non-phab, means

Mar 28 2019, 3:59 PM
rodgert abandoned D59125: Uglify utils.h functions.

Committed by other, non-phab, means

Mar 28 2019, 3:59 PM
rodgert abandoned D59192: Uglify par_backend namespace.

Committed by other, non-phab, means

Mar 28 2019, 3:57 PM
rodgert abandoned D59182: Uglify execution_impl.h identifiers.

Committed by other, non-phab, means

Mar 28 2019, 3:57 PM
rodgert abandoned D59124: Uglify brick_Xxx and pattern_Xxx functions.

Committed by other, non-phab, means

Mar 28 2019, 3:57 PM
rodgert abandoned D59117: Configuration needs to come from <execution>, <algorithm>, etc..

Committed by other, non-phab, means

Mar 28 2019, 3:56 PM
rodgert abandoned D59194: Uglifiy 'internal' namespace.

Committed by other, non-phab, means

Mar 28 2019, 3:55 PM
rodgert abandoned D59181: Uglify backend.

Committed by other, non-phab, means

Mar 28 2019, 3:55 PM

Mar 27 2019

rodgert abandoned D59110: Restructure existing test suite to follow libc++ standard layout.

Applied by "other means"

Mar 27 2019, 2:34 PM

Mar 26 2019

rodgert created D59856: [pstl] Restructure test suite to follow libc++ standard layout.
Mar 26 2019, 5:40 PM · Restricted Project

Mar 22 2019

rodgert added a comment to D59704: Changed buffer API to receive policy instance..

I don't see a big problem with this change per-se, but can you please expand on the bit where you say:

It may be necessary for the other back-ends, where dynamic memory is not available and custom memory allocator may be provided by a custom policy.

How would you somehow handle allocation through a custom policy?

Mar 22 2019, 2:10 PM

Mar 18 2019

rodgert updated the diff for D59110: Restructure existing test suite to follow libc++ standard layout.
  • Remove nested CMakeLists.txt in test/std, replace with glob-recurse
Mar 18 2019, 5:13 PM

Mar 12 2019

rodgert added a comment to D59110: Restructure existing test suite to follow libc++ standard layout.

To be clear about what I'd like: please do not add CMakeLists.txts all over the tree, instead keep relying on GLOB_RECURSE like my current test/CMakeLists.txt does.

Mar 12 2019, 10:01 AM
rodgert updated subscribers of D59194: Uglifiy 'internal' namespace.
Mar 12 2019, 9:51 AM
rodgert added a comment to D59194: Uglifiy 'internal' namespace.

@ldionne - When Intel added multiple new algorithm implementations, they did so without qualifying these calls. This led to a situation where some were and some weren't qualified.

Mar 12 2019, 9:47 AM

Mar 10 2019

rodgert created D59199: Uglify unseq_backend_simd.
Mar 10 2019, 5:11 PM
rodgert created D59194: Uglifiy 'internal' namespace.
Mar 10 2019, 2:19 PM
rodgert created D59192: Uglify par_backend namespace.
Mar 10 2019, 1:38 PM

Mar 9 2019

rodgert created D59182: Uglify execution_impl.h identifiers.
Mar 9 2019, 6:17 PM
rodgert created D59181: Uglify backend.
Mar 9 2019, 5:06 PM
rodgert added a comment to D59111: Deprecate non-CMake based build.

Do you want me to commit this?

Mar 9 2019, 2:42 PM · Restricted Project

Mar 7 2019

rodgert created D59125: Uglify utils.h functions.
Mar 7 2019, 7:09 PM
rodgert created D59124: Uglify brick_Xxx and pattern_Xxx functions.
Mar 7 2019, 6:11 PM
rodgert created D59122: Introduce forward declarations.
Mar 7 2019, 5:39 PM · Restricted Project
rodgert created D59117: Configuration needs to come from <execution>, <algorithm>, etc..
Mar 7 2019, 4:12 PM
rodgert created D59111: Deprecate non-CMake based build.
Mar 7 2019, 2:22 PM · Restricted Project
rodgert created D59110: Restructure existing test suite to follow libc++ standard layout.
Mar 7 2019, 2:18 PM

Mar 5 2019

rodgert abandoned D58852: Changes from integrating PSTL with libstdc++.

Will resubmit (as discussed) as a couple of smaller patches

Mar 5 2019, 1:53 PM
rodgert added a comment to D58852: Changes from integrating PSTL with libstdc++.
pstl_config.h is not required

Thomas, could you please clarify it?

The fact is the file contains significant macros, which are used in simd bricks, and the other useful macros.

Mar 5 2019, 6:44 AM

Mar 4 2019

rodgert added a comment to D58852: Changes from integrating PSTL with libstdc++.

Do you think you could split this change into smaller changes? Perhaps one that removes the support for the Makefile tests, one that introduces forward declarations, and one that renames the test suite files?

Mar 4 2019, 7:56 AM

Mar 1 2019

rodgert created D58852: Changes from integrating PSTL with libstdc++.
Mar 1 2019, 4:03 PM

Feb 21 2019

rodgert abandoned D58272: Add .gitignore.

I'm going to resubmit this as a set of smaller patches

Feb 21 2019, 4:13 PM

Feb 19 2019

rodgert updated the diff for D58272: Add .gitignore.
  • Introduce forward declaratoins
  • Deprecate non-cmake build and test suite
Feb 19 2019, 4:25 PM

Feb 15 2019

rodgert updated the diff for D58272: Add .gitignore.
  • Remove unused include
Feb 15 2019, 12:02 AM

Feb 14 2019

rodgert updated the diff for D58272: Add .gitignore.
  • Introduce forward definitions
Feb 14 2019, 11:28 PM
rodgert updated the diff for D58272: Add .gitignore.
  • Add ifdef guards for parallel overloads
Feb 14 2019, 10:23 PM
rodgert created D58272: Add .gitignore.
Feb 14 2019, 9:20 PM

Jan 22 2019

rodgert added inline comments to D56139: [pstl] Fix compile errors when PARALLEL_POLICIES is disabled.
Jan 22 2019, 10:18 AM

Jan 17 2019

rodgert added a comment to D56139: [pstl] Fix compile errors when PARALLEL_POLICIES is disabled.

Added a reply note to Mihail and Alexey's inline commentary on whether __PSTL_USE_PAR_POLICIES is how we really want to be controlling this.

Jan 17 2019, 10:53 AM
rodgert requested changes to D56139: [pstl] Fix compile errors when PARALLEL_POLICIES is disabled.

See inline notes on __PSTL_USE_PAR_POLICIES #ifdefs

Jan 17 2019, 10:44 AM

Jan 15 2019

rodgert reopened D56139: [pstl] Fix compile errors when PARALLEL_POLICIES is disabled.
Jan 15 2019, 1:02 PM
rodgert added a comment to D56139: [pstl] Fix compile errors when PARALLEL_POLICIES is disabled.

See inline comment

Jan 15 2019, 1:00 PM

Jan 14 2019

rodgert abandoned D56676: Introduce forward definitions.

Ignore this version, Intel changed the definitions of most (all?) of these entry points in the version committed to LLVM, will resubmit after fixing signatures

Jan 14 2019, 5:07 PM
rodgert created D56676: Introduce forward definitions.
Jan 14 2019, 10:47 AM