Page MenuHomePhabricator

C API: functions to get mask of a ShuffleVector
ClosedPublic

Authored by cdisselkoen on Sep 23 2020, 4:15 PM.

Details

Summary

This commit fixes a regression (from LLVM 10 to LLVM 11 RC3) in the LLVM C API.

Previously, commit 1ee6ec2bf removed the mask operand from the
ShuffleVector instruction, storing the mask data separately in the instruction
instead; this reduced the number of operands of ShuffleVector from 3 to 2.
AFAICT, this change unintentionally caused a regression in the LLVM C API.
Specifically, it is no longer possible to get the mask of a ShuffleVector
instruction through the C API. This patch introduces new functions which
together allow a C API user to get the mask of a ShuffleVector instruction,
restoring the functionality which was previously available through
LLVMGetOperand().

This patch also adds tests for this change to the llvm-c-test executable, which
involved adding support for InsertElement, ExtractElement, and ShuffleVector
itself (as well as constant vectors) to echo.cpp. Previously, vector
operations weren't tested at all in echo.ll.

I also fixed some typos in comments and help-text nearby these changes, which I
happened to spot while developing this patch. Since the typo fixes are technically
unrelated other than being in the same files, I'm happy to take them out if
you'd rather they not be included in the patch.

I'm not sure who the appropriate reviewer / code owner is for this diff,
so I'm taking a guess and adding @arsenm since they reviewed D67132 which
also touched the C API. If this is wrong, feel free to let me know.

Diff Detail

Event Timeline

cdisselkoen created this revision.Sep 23 2020, 4:15 PM
Herald added a project: Restricted Project. · View Herald Transcript
cdisselkoen requested review of this revision.Sep 23 2020, 4:15 PM
cdisselkoen edited the summary of this revision. (Show Details)Sep 23 2020, 4:51 PM

some clang-tidy suggestions

whoops that isn't how arcanist works

These changes all make sense, I think.

llvm/tools/llvm-c-test/echo.cpp
812

Any particular reason you aren't using a SmallVector here?

cdisselkoen added inline comments.Sep 24 2020, 12:45 PM
llvm/tools/llvm-c-test/echo.cpp
812

I guess it was just because conceptually in my mind this is C code and not C++ code (it consumes the C API). Happy to use SmallVector if you prefer

Use SmallVector rather than array

cdisselkoen marked an inline comment as done.Sep 24 2020, 1:01 PM
cdisselkoen added inline comments.
llvm/tools/llvm-c-test/echo.cpp
812

Updated to use SmallVector

efriedma added inline comments.Sep 24 2020, 2:10 PM
llvm/tools/llvm-c-test/echo.cpp
812

Did you mean to use push_back?

cdisselkoen added inline comments.Sep 24 2020, 4:32 PM
llvm/tools/llvm-c-test/echo.cpp
812

Uh, yes I did, whoops. Weird, I thought I ran tests locally but I must have not done that

fix: use push_back

efriedma accepted this revision.Sep 24 2020, 7:38 PM

LGTM. Thanks for taking the time to improve llvm-c-test along with this patch.

I'll commit the patch for you; how do you want to be credited in the "author" line?

This revision is now accepted and ready to land.Sep 24 2020, 7:38 PM

This is my first contribution to LLVM, I'm not sure what the typical practice is, but if it's a normal Git commit, then Craig Disselkoen <craigdissel@gmail.com> is fine. Thanks for the review!

Also, again I'm new here, but since this fixes a regression, if there's any possibility of landing this on the 11.x branch in addition to master, I'd appreciate it. I know it's probably too late for 11.0.0.

hans added a comment.Sep 25 2020, 4:35 AM

This is my first contribution to LLVM, I'm not sure what the typical practice is, but if it's a normal Git commit, then Craig Disselkoen <craigdissel@gmail.com> is fine. Thanks for the review!

Also, again I'm new here, but since this fixes a regression, if there's any possibility of landing this on the 11.x branch in addition to master, I'd appreciate it. I know it's probably too late for 11.0.0.

We try not to change the C API in .1 releases, so it either needs to wait for 12, or go into 11.0.0. Since we'll need to do another release candidate for 11, I'd be okay with merging this since it seems pretty safe (assuming Eli agrees).

This revision was landed with ongoing or failed builds.Sep 25 2020, 4:01 PM
This revision was automatically updated to reflect the committed changes.

@hans I think this is okay to land in 11.0.

CodaFi added a subscriber: CodaFi.Sep 25 2020, 10:18 PM
CodaFi added inline comments.
llvm/lib/IR/Core.cpp
3966

Can we add a getter for this instead? We should avoid magic numbers in the C API - it doesn’t tend to stay in sync with the C++ otherwise

cdisselkoen added inline comments.Sep 26 2020, 10:41 AM
llvm/lib/IR/Core.cpp
3966

Actually, as I was looking into this, I realized that we can just write const int LLVMUndefMaskElem = UndefMaskElem; here and it works. UndefMaskElem is just a constant available in the Instructions.h header.

Since this has already landed on master, should I open a new diff with this change? Or still modify this diff?

CodaFi added inline comments.Sep 26 2020, 11:42 AM
llvm/lib/IR/Core.cpp
3966

To both questions

  1. Please don’t mix C and C++ headers. It may happen to work for you locally now, but there’s no guarantee Instructions.h won’t break everybody in the future. Please make an explicit getter.
  1. You should submit another revision for review here.
cdisselkoen added inline comments.Sep 26 2020, 11:53 AM
llvm/lib/IR/Core.cpp
3966

Please don't mix C and C++ headers ... there's no guarantee Instructions.h won't break everybody in the future

Apologies for my lack of knowledge (C++ is not my primary language), but could you explain what the concern is here? Instructions.h (and other C++ headers) are already included in Core.cpp, so the line above works without including any new headers. And we wouldn't have to change Core.h which is the actual C header -- in that header the current diff just has a declaration extern const int LLVMUndefMaskElem;, and we could keep that declaration the same.

CodaFi added inline comments.Sep 26 2020, 11:58 AM
llvm/lib/IR/Core.cpp
3966

The C bindings are meant to present a pure C view of a C++ API. It is only natural then that we *implement* these bindings in C++, but we express the interface to those bindings in C. But despite their names these languages are strictly incompatible. If I were to build a C-only translation unit with a C-only compiler that imported Instructions.h, I would not succeed. There are clients like the OCaml bindings, go bindings, and my own set of Swift bindings that would also break upon trying to import a C++ header in a C context.

CodaFi added inline comments.Sep 26 2020, 12:09 PM
llvm/lib/IR/Core.cpp
3966

The C API also has additional stability commitments that the C++ API roundly does not make. In order to carry out those guarantees, we make heavy use of abstraction. Notice that every API here that takes LLVM objects takes them in memory through bare typedefs of opaque pointers. Here, we have an implementation detail of the C++ API leaking to clients through this constant. If tomorrow somebody were to add support for a host that used some other value for this constant, we would be hosed by the lack of abstraction here.

cdisselkoen added inline comments.Sep 26 2020, 12:23 PM
llvm/lib/IR/Core.cpp
3966

The C bindings are meant to present a pure C view of a C++ API ... If I were to build a C-only translation unit with a C-only compiler that imported Instructions.h, I would not succeed. ... OCaml bindings, go bindings, and my own set of Swift bindings would also break

Sure, totally understood on this. (I also maintain these Rust bindings for LLVM which depend on the C API.) What I don't understand is what about this change would require C consumers to import Instructions.h. Is that somehow implied by the use of extern const int in the C header? The code seems to compile for me (and pass tests) without importing Instructions.h in the C header - we're only referencing Instructions.h in the C++ implementation of the bindings.

If tomorrow somebody were to add support for a host that used some other value for this constant

Is Core.cpp target-specific? I don't understand how different hosts could have different values for this constant. Again, I'm not the most familiar with C++ so this is probably me misunderstanding something.

cdisselkoen reopened this revision.Sep 26 2020, 12:27 PM

Reopened per comments from @CodaFi

This revision is now accepted and ready to land.Sep 26 2020, 12:27 PM

Use UndefMaskElem instead of magic-number constant

llvm/lib/IR/Core.cpp
3966

In case it makes the discussion more clear, I've updated the diff with my proposed change

CodaFi added inline comments.Sep 26 2020, 3:18 PM
llvm/lib/IR/Core.cpp
3966

You do *not* need additional includes in Core.h to write a getter.

unsigned LLVMGetUndefMaskElem(void);
CodaFi added inline comments.Sep 26 2020, 3:21 PM
llvm/lib/IR/Core.cpp
3966

Apologies, that should be a signed integer.

Regardless, this version of affairs will break existing clients. The constant initializer here is a constexpr declaration, which is not a construct in C. The existing LLVM tests use C++ translation units which can mask the failure here. That’s why I’m pushing for a getter.

CodaFi added inline comments.Sep 26 2020, 3:28 PM
llvm/lib/IR/Core.cpp
3966

I'm going to submit a revision that corrects this and tag you as a reviewer. I find it easier to read code than talk about it.

CodaFi closed this revision.Sep 26 2020, 3:29 PM

This has already been committed, it should remain closed.

cdisselkoen added inline comments.Sep 26 2020, 3:45 PM
llvm/lib/IR/Core.cpp
3966

You do *not* need additional includes in Core.h to write a getter.

Absolutely agreed. But you don't need additional includes in Core.h to have an extern const int declaration, either?

The constant initializer here is a constexpr declaration, which is not a construct in C

Again, I'm sure you're right, but just for my own edification - I'm confused why a constexpr declaration in the _C++ implementation_ is different than, say, a getter implemented in C++? Neither of those are valid C; we can't compile Core.cpp in pure C either way. There wouldn't be any constexpr in Core.h -- just an extern const declaration -- so wouldn't the linker do the right thing? And wouldn't any code including Core.h remain valid C, since there is no constexpr there?

I'm not arguing that a getter is bad, and I'm happy to make it a getter. I'm just trying to understand the problem better here. Apologies as I know my lack of C++ knowledge is showing.

hans added a comment.Sep 28 2020, 3:39 AM

For those following along, the follow-up patch was https://reviews.llvm.org/D88367

@hans I think this is okay to land in 11.0.

Thanks. I've pushed this and the follow-up to 11.x as 9e367bd69b0d2523237e204b43301e59a5badb29 and 293924973057e33fcc63521f582bb9fd41e60cc4, respectively.