Page MenuHomePhabricator

[mlir] Make locations required when adding/creating block arguments
ClosedPublic

Authored by rriddle on Jan 18 2022, 6:29 PM.

Details

Summary

BlockArguments gained the ability to have locations attached a while ago, but they
have always been optional. This goes against the core tenant of MLIR where location
information is a requirement, so this commit updates the API to require locations.

Fixes #53279

Diff Detail

Event Timeline

rriddle created this revision.Jan 18 2022, 6:29 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: sjarus. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
rriddle requested review of this revision.Jan 18 2022, 6:29 PM
lattner accepted this revision.Jan 19 2022, 12:09 PM

Nice, this is a great step, thank you for doing this!

What is our policy for the C APIs, are we allowed to fix them, or do we have to add a new entrypoint?

This revision is now accepted and ready to land.Jan 19 2022, 12:09 PM

I don't remember seeing a discussion about "freezing" the C API so far, we stated a "best effort" intent originally. However external users may have started depending on them "de facto", skipping the discussion on getting a better contract first...
Adding trampoline if often trivial, but having to maintain suffixed numbered variant of the symbols isn't very appealing :(

Makes sense, it should definitely be a different discussion from this patch of course. My preference is to keep the C API barnacle free if we can, I simply don't know if anyone cares about compatibility here. Before fixing the C API it is probably best to discuss on the forum etc.

Nice, this is a great step, thank you for doing this!

What is our policy for the C APIs, are we allowed to fix them, or do we have to add a new entrypoint?

AFAIK (and as Mehdi mentions) we try to maintain them as-is in a best effort manor, but we haven't yet committed to any kind of stability policy. Just went ahead and updated them here.

Makes sense, it should definitely be a different discussion from this patch of course. My preference is to keep the C API barnacle free if we can, I simply don't know if anyone cares about compatibility here. Before fixing the C API it is probably best to discuss on the forum etc.

Worth a discussion, but I think we are still in the phase of aspiring to low churn on the C API vs having a hard line. Personally, I think there is a lot of accumulated debt that is being flushed here and am +1 on actually flushing the queue and getting to a better island of stability from then on.

Nice, this is a great step, thank you for doing this!

What is our policy for the C APIs, are we allowed to fix them, or do we have to add a new entrypoint?

AFAIK (and as Mehdi mentions) we try to maintain them as-is in a best effort manor, but we haven't yet committed to any kind of stability policy. Just went ahead and updated them here.

If we were to have such a policy for stability, I would rather it be something along the lines of "we can remove entry-points when needed but we should avoid changing the signature of existing entry-points" (basically, we eventually want link/symbol resolution errors, not weird signature mismatches). But I don't think we are even at that point yet in the development process. I vote for just fixing it.