[PATCH] D88190: C API: functions to get mask of a ShuffleVector

Robert Widmann via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Sep 26 15:28:22 PDT 2020


CodaFi added inline comments.


================
Comment at: llvm/lib/IR/Core.cpp:3966
+}
+const int LLVMUndefMaskElem =
+    -1; // not actually accessible as ShuffleVectorInst::UndefMaskElem, so we
----------------
CodaFi wrote:
> CodaFi wrote:
> > cdisselkoen wrote:
> > > cdisselkoen wrote:
> > > > CodaFi wrote:
> > > > > CodaFi wrote:
> > > > > > cdisselkoen wrote:
> > > > > > > CodaFi wrote:
> > > > > > > > cdisselkoen wrote:
> > > > > > > > > CodaFi wrote:
> > > > > > > > > > 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
> > > > > > > > > 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?
> > > > > > > > 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.
> > > > > > > > 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.
> > > > > > 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.
> > > > > 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.
> > > > > 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](https://github.com/cdisselkoen/llvm-ir).)  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.
> > > In case it makes the discussion more clear, I've updated the diff with my proposed change
> > You do *not* need additional includes in Core.h to write a getter.
> > 
> > ```
> > unsigned LLVMGetUndefMaskElem(void);
> > ```
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88190/new/

https://reviews.llvm.org/D88190



More information about the llvm-commits mailing list