C API: functions to get mask of a ShuffleVector

[cdisselkoen]

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.

[cdisselkoen]

some clang-tidy suggestions

[cdisselkoen]

whoops that isn't how arcanist works

[efriedma]

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]

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

[cdisselkoen]

Use SmallVector rather than array

[cdisselkoen]

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

812

Updated to use SmallVector

[efriedma]

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

812

Did you mean to use push_back?

[cdisselkoen]

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

[cdisselkoen]

fix: use push_back

[efriedma]

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?

[cdisselkoen]

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 <[e-mail]> 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]

In D88190#2294181, @cdisselkoen wrote:

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 <[e-mail]> 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).

[efriedma]

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

[CodaFi]

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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]

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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]

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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.

2. You should submit another revision for review here.

[cdisselkoen]

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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]

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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]

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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]

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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 per comments from @CodaFi

[cdisselkoen]

Use UndefMaskElem instead of magic-number constant

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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

[CodaFi]

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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

unsigned LLVMGetUndefMaskElem(void);

[CodaFi]

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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]

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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]

This has already been committed, it should remain closed.

[cdisselkoen]

llvm/lib/IR/Core.cpp

3966 ↗

(On Diff #294448)

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]

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

In D88190#2296199, @efriedma wrote:

@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.

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

switch(Op) {
         LLVMValueRef V = CloneValue(LLVMGetOperand(Src, 1));
         LLVMValueRef Index = CloneValue(LLVMGetOperand(Src, 2));
         Dst = LLVMBuildInsertElement(Builder, Agg, V, Index, Name);
         break;
       }
       case LLVMShuffleVector: {
         LLVMValueRef Agg0 = CloneValue(LLVMGetOperand(Src, 0));
         LLVMValueRef Agg1 = CloneValue(LLVMGetOperand(Src, 1));
-        unsigned num_mask_elts = LLVMGetNumMaskElements(Src);
-        if (num_mask_elts > 8)
+        unsigned NumMaskElts = LLVMGetNumMaskElements(Src);
+        if (NumMaskElts > 8)
           report_fatal_error(
               "ShuffleVector: Expected no more than 8 mask elements");
-        LLVMValueRef mask_elts[8];
-        for (unsigned i = 0; i < num_mask_elts; i++) {
-          int val = LLVMGetMaskValue(Src, i);
-          if (val == LLVMUndefMaskElem) {
-            mask_elts[i] = LLVMGetUndef(LLVMInt64Type());
+        LLVMValueRef MaskElts[8];
+        for (unsigned i = 0; i < NumMaskElts; i++) {
+          int Val = LLVMGetMaskValue(Src, i);
+          if (Val == LLVMUndefMaskElem) {
+            MaskElts[i] = LLVMGetUndef(LLVMInt64Type());
           } else {
-            mask_elts[i] = LLVMConstInt(LLVMInt64Type(), val, true);
+            MaskElts[i] = LLVMConstInt(LLVMInt64Type(), Val, true);
           }
         }
-        LLVMValueRef Mask = LLVMConstVector(&mask_elts[0], num_mask_elts);
+        LLVMValueRef Mask = LLVMConstVector(&MaskElts[0], NumMaskElts);
         Dst = LLVMBuildShuffleVector(Builder, Agg0, Agg1, Mask, Name);
         break;
       }
       case LLVMFreeze: {
         LLVMValueRef Arg = CloneValue(LLVMGetOperand(Src, 0));
         Dst = LLVMBuildFreeze(Builder, Arg, Name);
         break;
       }