Page MenuHomePhabricator

GeorgeLyon (George)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 20 2020, 10:14 AM (6 w, 3 d)

Recent Activity

Yesterday

GeorgeLyon closed D92613: Use MlirStringRef in StandardAttributes.h.

Accidentally committed without "Differential" log: https://github.com/llvm/llvm-project/commit/5f65c4a8e6a9fbcbf45ff4cdf0b4815795dd4845

Thu, Dec 3, 4:25 PM · Restricted Project
GeorgeLyon added a comment to D92613: Use MlirStringRef in StandardAttributes.h.

Whoops, I accidentally pushed without the "Differential" comment... anything I should do to clean this up?

Thu, Dec 3, 4:14 PM · Restricted Project
GeorgeLyon updated the diff for D92613: Use MlirStringRef in StandardAttributes.h.

Fix comments

Thu, Dec 3, 4:01 PM · Restricted Project
GeorgeLyon abandoned D92279: Add SCF Dialect C API.
Thu, Dec 3, 3:26 PM · Restricted Project
GeorgeLyon updated the summary of D92613: Use MlirStringRef in StandardAttributes.h.
Thu, Dec 3, 3:24 PM · Restricted Project
GeorgeLyon requested review of D92613: Use MlirStringRef in StandardAttributes.h.
Thu, Dec 3, 3:23 PM · Restricted Project

Sun, Nov 29

GeorgeLyon accepted D92292: [mlir][CAPI] Convert the rest of the API int -> bool..

LGTM. Thanks for doing this!

Sun, Nov 29, 6:39 PM · Restricted Project
GeorgeLyon added a comment to D92193: Use bool in place of int for boolean things in the C API.

Only the helpers that are defined in the header are static inline. I think that the intent was to not exposing them in libMLIR.so and have them inlined in the client. I am not sure this is valid in C though: they has to be an external definition I believe and this would break if a client was taking the address.

Sun, Nov 29, 4:34 PM · Restricted Project
GeorgeLyon added a comment to D92279: Add SCF Dialect C API.

The code in this revision should all be part of the TableGen generated code and not manually written.

Sun, Nov 29, 4:22 PM · Restricted Project
GeorgeLyon added a comment to D92279: Add SCF Dialect C API.

That said, I think it would be nice to include a second dialect in the bindings as we work our way to a better system (and I need it for my test cases :-)

Sun, Nov 29, 12:11 PM · Restricted Project
GeorgeLyon added a comment to D92279: Add SCF Dialect C API.

I very much agree @stellaraccident. I would even go one step further and say there should be an mlirGetDialectByNamespace(MlirStringRef) function to return this structure, which we can enable using static constructors (even though they are evil, this would be a very nice way to not have to create a CAPI for every dialect). For the record, in the Swift bindings I use "Dialect" to mean what you refer to as "Dialect Registration" and "Dialect Instance" to refer to the thing returned by getOrLoad. I also don't currently understand how Dialect Instances are useful (and why we can't just have "Dialect Registration" be the only thing (hence "Dialect")

Sun, Nov 29, 12:10 PM · Restricted Project
GeorgeLyon added reviewers for D92279: Add SCF Dialect C API: mehdi_amini, ftynse, stellaraccident.
Sun, Nov 29, 10:16 AM · Restricted Project
GeorgeLyon requested review of D92279: Add SCF Dialect C API.
Sun, Nov 29, 10:15 AM · Restricted Project
GeorgeLyon added a comment to D92193: Use bool in place of int for boolean things in the C API.

I keep getting this error saying that the Debian build failed, though I don't see any errors:

Testing Time: 121.11s
  Unsupported      :   584
  Passed           : 40988
  Expectedly Failed:   174
Sun, Nov 29, 10:10 AM · Restricted Project
GeorgeLyon updated the diff for D92252: Use `const` for array pointers in `StandardTypes.h`.

Add some missing consts

Sun, Nov 29, 9:46 AM · Restricted Project
GeorgeLyon updated the diff for D92193: Use bool in place of int for boolean things in the C API.

Let's give Debian another chance...

Sun, Nov 29, 9:02 AM · Restricted Project
GeorgeLyon updated the diff for D92252: Use `const` for array pointers in `StandardTypes.h`.

Rebasing again

Sun, Nov 29, 9:00 AM · Restricted Project

Sat, Nov 28

GeorgeLyon updated the diff for D92252: Use `const` for array pointers in `StandardTypes.h`.

Rebasing to re-run CI

Sat, Nov 28, 10:10 AM · Restricted Project
GeorgeLyon updated the diff for D92193: Use bool in place of int for boolean things in the C API.

Rebasing to re-run CI

Sat, Nov 28, 10:08 AM · Restricted Project

Fri, Nov 27

GeorgeLyon updated the diff for D92252: Use `const` for array pointers in `StandardTypes.h`.

Try again after rebase

Fri, Nov 27, 4:57 PM · Restricted Project
GeorgeLyon updated the diff for D92252: Use `const` for array pointers in `StandardTypes.h`.

Fix diff

Fri, Nov 27, 4:55 PM · Restricted Project
GeorgeLyon retitled D92252: Use `const` for array pointers in `StandardTypes.h` from More const pointers to Use `const` for array pointers in `StandardTypes.h` .
Fri, Nov 27, 4:52 PM · Restricted Project
GeorgeLyon requested review of D92252: Use `const` for array pointers in `StandardTypes.h`.
Fri, Nov 27, 4:51 PM · Restricted Project
GeorgeLyon added a comment to D92193: Use bool in place of int for boolean things in the C API.

I'm guessing it is safe to ignore the unused function warnings?

Fri, Nov 27, 1:07 PM · Restricted Project
GeorgeLyon updated the summary of D92193: Use bool in place of int for boolean things in the C API.
Fri, Nov 27, 1:06 PM · Restricted Project

Thu, Nov 26

GeorgeLyon added a comment to D92192: Fix appending-zero issues.

Sorry I abandoned this because the situation I thought I was able to reproduce, stopped reproducing. I think the underlying issue was unrelated

Thu, Nov 26, 8:23 PM · Restricted Project
GeorgeLyon retitled D92193: Use bool in place of int for boolean things in the C API from Use bool in place of int for boolean things to Use bool in place of int for boolean things in the C API.
Thu, Nov 26, 12:34 PM · Restricted Project
GeorgeLyon updated the summary of D92193: Use bool in place of int for boolean things in the C API.
Thu, Nov 26, 12:33 PM · Restricted Project
GeorgeLyon requested review of D92193: Use bool in place of int for boolean things in the C API.
Thu, Nov 26, 12:31 PM · Restricted Project
GeorgeLyon abandoned D92192: Fix appending-zero issues.
Thu, Nov 26, 12:08 PM · Restricted Project
GeorgeLyon requested review of D92192: Fix appending-zero issues.
Thu, Nov 26, 12:02 PM · Restricted Project

Tue, Nov 24

GeorgeLyon updated subscribers of D92007: [mlir][Python] Sync Python bindings with C API MlirStringRef modification..

The difference is that all the consumers of the C++ API are C++, so it is pretty straightforward to update when making breaking changes even if the surface is greater. Language bindings will live at least partially in their own ecosystem and requiring folks who work on the C API to understand each individual ecosystem may be too onerous, even if the surface area is smaller. To be clear, I’m just saying there is a tradeoff here, not advocating for us to decide on a particular point on that spectrum. I agree with Stella that we can wait until this becomes a bigger issue and then have a more formal discussion (hopefully with more data in the form of diverse language bindings).

Tue, Nov 24, 11:05 AM · Restricted Project
GeorgeLyon added a comment to D92007: [mlir][Python] Sync Python bindings with C API MlirStringRef modification..

Small process question: I authored the change to us MlirStringRef instead of const char *, what is the policy about breaking changes in the C API? I think it is likely unreasonable to expect any change in the C API to fix all in-tree language bindings, but since the Python bindings are in-tree, these types of changes can break them. I'm might be the only person working on non-Python bindings here, so I may be biased :)

Tue, Nov 24, 9:26 AM · Restricted Project

Mon, Nov 23

GeorgeLyon updated the diff for D91905: Use MlirStringRef throughout the C API.

Some more formatting fixes

Mon, Nov 23, 12:43 PM · Restricted Project
GeorgeLyon abandoned D91991: Some more formatting fixes.
Mon, Nov 23, 12:42 PM · Restricted Project
GeorgeLyon removed rG LLVM Github Monorepo as the repository for D91991: Some more formatting fixes.
Mon, Nov 23, 12:42 PM · Restricted Project
GeorgeLyon requested review of D91991: Some more formatting fixes.
Mon, Nov 23, 12:41 PM · Restricted Project
GeorgeLyon added a comment to D91905: Use MlirStringRef throughout the C API.

Sorry for the spam, I am still learning my way around arc diff. This should be ready now though.

Mon, Nov 23, 11:48 AM · Restricted Project
GeorgeLyon retitled D91905: Use MlirStringRef throughout the C API from Allow Location to be created with a string ref to Use MlirStringRef throughout the C API.
Mon, Nov 23, 11:48 AM · Restricted Project
GeorgeLyon updated the diff for D91905: Use MlirStringRef throughout the C API.

Use MlirStringRef throught the C API

Mon, Nov 23, 11:46 AM · Restricted Project
GeorgeLyon updated the diff for D91905: Use MlirStringRef throughout the C API.

Use MlirStringRef for all strings in the C API

Mon, Nov 23, 11:43 AM · Restricted Project
GeorgeLyon updated the diff for D91905: Use MlirStringRef throughout the C API.

Use MlirStringRef for all strings in the C API

Mon, Nov 23, 11:40 AM · Restricted Project
GeorgeLyon updated the diff for D91905: Use MlirStringRef throughout the C API.

Fix formatting

Mon, Nov 23, 11:37 AM · Restricted Project
GeorgeLyon updated the diff for D91905: Use MlirStringRef throughout the C API.

Use MlirStringRef throughout the API.

Mon, Nov 23, 11:34 AM · Restricted Project

Sat, Nov 21

GeorgeLyon added a comment to D91905: Use MlirStringRef throughout the C API.

To be perfectly clear, I agree that we should use MlirStringRef throughout, but if we decide that we should do that across the API in one fell swoop (I’m happy to author that PR if folks are on board).

Sat, Nov 21, 8:57 PM · Restricted Project

Fri, Nov 20

GeorgeLyon updated the diff for D91905: Use MlirStringRef throughout the C API.

Removing spurious formatting changes

Fri, Nov 20, 6:50 PM · Restricted Project
GeorgeLyon updated the summary of D91905: Use MlirStringRef throughout the C API.
Fri, Nov 20, 6:48 PM · Restricted Project
GeorgeLyon requested review of D91905: Use MlirStringRef throughout the C API.
Fri, Nov 20, 6:41 PM · Restricted Project
GeorgeLyon added a comment to D91738: Add userData to the diagnostic handler C API.

I have worked with many C APIs in a number of languages and the attitude of "just do the memory management bookkeeping in the host language" has created some very unfortunate complexity in the bindings and more than a few bugs. This is a very low-cost change for the C bindings, and doesn't add much complexity for language bindings that don't care about this functionality (they can just pass NULL to the extra arguments). I think this is also a win from a self-documentation standpoint: before, I would need to go and figure out what are the different ways diagnostic handlers can be unregistered to understand the semantics. I would also need to worry about concurrency, specifically whether or not my handler is guaranteed to not be called after detach returns. I could read the code to figure this out (it turns out, the answer is "no") but I think it is better to minimize this sort of spelunking and just say "we will call your handler and when we are done calling your handler we will call your callback".

Fri, Nov 20, 10:25 AM · Restricted Project
GeorgeLyon updated the diff for D91738: Add userData to the diagnostic handler C API.

Address feedback

Fri, Nov 20, 10:25 AM · Restricted Project

Thu, Nov 19

GeorgeLyon added a comment to D91738: Add userData to the diagnostic handler C API.

If a host language is registering dialect handlers that may need to be cleaned up (for instance, via a reference count decrement), it theoretically _could_ keep a list of attached handlers and manually release them on detach or when the context is destroyed. deleteUserData is a simple way to eliminate this bit of bookkeeping and make "ownership" a bit more formal in this case, namely the context is responsible for telling you when it is done using the handler (and thus, the userData). This way, if we hypothetically add a setHandlers method which takes an array of handles and _replaces_ the current handler stack, then the host language wouldn't need to account for this, deleteUserData would just be called when the handler is no longer in use.

Thu, Nov 19, 4:36 PM · Restricted Project
GeorgeLyon added a comment to D91738: Add userData to the diagnostic handler C API.

@mehdi_amini I added one more bit of functionality here, so you may want to take another look.

Thu, Nov 19, 2:17 PM · Restricted Project
GeorgeLyon updated the diff for D91738: Add userData to the diagnostic handler C API.

Added deleteUserData callback

Thu, Nov 19, 2:16 PM · Restricted Project
GeorgeLyon accepted D91822: [mlir] Add missing const * updates in StandardAttributes.

Whoops, thank you for catching this... these must have disappeared when I was fixing MLIR_CAPI_EXPORTED-related merge conflicts

Thu, Nov 19, 1:54 PM · Restricted Project

Wed, Nov 18

GeorgeLyon updated the summary of D91738: Add userData to the diagnostic handler C API.
Wed, Nov 18, 1:22 PM · Restricted Project
GeorgeLyon updated the summary of D91740: Make array pointers in the CAPI const.
Wed, Nov 18, 1:20 PM · Restricted Project
GeorgeLyon requested review of D91740: Make array pointers in the CAPI const.
Wed, Nov 18, 1:18 PM · Restricted Project
GeorgeLyon requested review of D91738: Add userData to the diagnostic handler C API.
Wed, Nov 18, 1:07 PM · Restricted Project