In D123914#3470145, @jberdine wrote:

LGTM, though I would prefer if docstrings were added for each of the functions added to llvm_debuginfo.mli.

Add inline docs for the newly added functions in the .mli file.

@krzysz00 A few months back, I had attempted a generic value-range analysis in MLIR (see https://github.com/vaivaswatha/llvm-project/pull/1). While I had (privately) requested for initial design feedback from @rriddle , I couldn't get around to post an official patch on it.

This change looks fine to me.

@rriddle I've added the debug statement I had locally. If this looks fine to you, I'll go ahead and push the change.

Incorporating review comments.

In D99475#2663806, @jberdine wrote:

See https://github.com/jberdine/llvm-project/tree/ocaml for this stack on github.

For the init code, I just mean to add that Gc.set call somewhere in your code that uses the LLVM bindings before you do much with Llvm.

In D99475#2661044, @jberdine wrote:

I have been testing this stack of diffs with a sort of stress test that involves running a system that consumes bitcode and uses the ocaml api to traverse and translate it. The testing has used a range of OCaml GC settings in order to make the GC work much harder and trigger much more often than usual (e.g. Gc.set {(Gc.get ()) with minor_heap_size= 1024; space_overhead= 20}). The reasoning here is that if the bindings are not playing nice with the GC, then there is a higher chance to uncover such issues if the GC runs much more often. This testing has not revealed any issues.

@vaivaswatha if it is not too much overhead in your application, it would be helpful if you could arc patch these diffs, add a call to Gc.set as above in your initialization code, and just run your application to see if there are any crashes.

Similar to my comment in https://reviews.llvm.org/D99472, I don't understand this a 100%, but it seems sane to me.

As far as I can see, this looks fine, but I'm far from an expert on OCaml internals. It would be greatly useful if someone else who understands OCaml internals can take a look, but I don't know if it's easy finding such a reviewer here. So if you're confident and the code has been tested well on your end, I suppose it's okay to commit this.

This fixes a potentially hard-to-discover bug ! The last thing that a user of the OCaml API would think of is "oh this function uses Store_field to initialize fields of blocks allocated with caml_alloc_small and that could lead to a hard to trace GC bug".

Option seems to not work on the buildbot machines. So I ended up replacing that with a match

Remove #include <stdio.h> that was inadvertently added.

Whitespace changes

Use mlsize_t for cstr_to_string as suggested in the review.

In D90831#2630893, @vaivaswatha wrote:

In D90831#2630862, @vaivaswatha wrote:

@jberdine: after commit, I found this failure emailed to me llvm_ocaml.c:function cstr_to_string: error: undefined reference to 'caml_alloc_initialized_string'. Do you happen to know how I can fix it, because it works on my Ubuntu 20.04 system. What OS do you have? (because you reported that it worked too). A link to one of the build fails I got over email: https://lab.llvm.org/buildbot/#/builders/16/builds/7919

I've now switched to using the following code instead in cstr_to_string:

if (Str) {
  String = caml_alloc_string(Len);
  memcpy(String_val(Str), Str, Len);
} else {
  String = caml_alloc_string(0);
}

In D90831#2630862, @vaivaswatha wrote:

@jberdine: after commit, I found this failure emailed to me llvm_ocaml.c:function cstr_to_string: error: undefined reference to 'caml_alloc_initialized_string'. Do you happen to know how I can fix it, because it works on my Ubuntu 20.04 system. What OS do you have? (because you reported that it worked too). A link to one of the build fails I got over email: https://lab.llvm.org/buildbot/#/builders/16/builds/7919

@jberdine: after commit, I found this failure emailed to me llvm_ocaml.c:function cstr_to_string: error: undefined reference to 'caml_alloc_initialized_string'. Do you happen to know how I can fix it, because it works on my Ubuntu 20.04 system. What OS do you have? (because you reported that it worked too). A link to one of the build fails I got over email: https://lab.llvm.org/buildbot/#/builders/16/builds/7919

In D90831#2629836, @jberdine wrote:

Thanks @vaivaswatha for dealing with all my nitpicking! Yes, I've made a final pass over this and it looks good. I'd recheck the build before committing given the CI build failure.

Thank you @jberdine for the detailed review and helping through this patch. Shall I commit it?

I've updated the patch addressing some of your comments. In particular, I didn't add a reference to the C-API to the functions where I had already provided the explanations inline (and the C-API comment doesn't add more value).

In D98695#2628657, @rengolin wrote:

Actually, this is mentioned in a different document: https://www.llvm.org/docs/Phabricator.html#committing-a-change

I agree this isn't perfect and I think your change is worth doing. Can you also add a link to the URL above?

Something like "add a link to its review page, as shown in [Phab link]".

Thanks!

Add missing fullstop.

Link to Phabricator page that mentions the same topic.

Fix clang-tidy warning on header guard name convention.

@jberdine: I have now updated the patch with documentation for the newly added APIs, inline. Further:

Thank you very much @jberdine . I've incorporated all of your suggestions with this latest diff. I haven't yet added the documentation and will do that by tomorrow. Please have a final look at the latest changes so that with my update tomorrow it should only be comments over each function and (hopefully) no code changes.

In D90831#2625375, @vaivaswatha wrote:

@jberdine I have addressed all your comments, except for the one on stricter type aliases for Metadata.

Is this what you were working toward and ran into problems with?

No, I only attempted something simpler along the lines of your experimental change. Nothing so much as having conversion functions between them.

I need to think what's the best way to model the C++ type hierarchy for Metadata here in OCaml. For example, there is llvalue and llbasicblock already in Llvml.ml, but they just rely on, internally in Core.cpp, cast<> throwing a runtime error (at least that's what I understood of it).

that would optimally be implemented with something like a dyn_cast

We don't have dyn_cast available in the C-API (I couldn't find any uses of it). So we might need to reestablish the subtyping from scratch using the MetadataKind enum values. I'm not sure that will be easy to maintain though. I can give it a try.

The types that we'll need to model include, at least the following:

Address all but one review comment.

@jberdine I have addressed all your comments, except for the one on stricter type aliases for Metadata.

Thank you very much @jberdine for the review, especially for the ready to incorporate changes in your fork (which I just cherry-picked).

It's been more than a week, so a gentile reminder ping !

@jberdine just pinged me about his earlier attempt to do this, here's a reference: https://reviews.llvm.org/D60902.

Mention both CMAKE_INSTALL_PREFIX and LLVM_OCAML_INSTALL_PATH in the OCaml bindings README

This is already fixed on trunk.

Updating based on review comments

Updating based on review comments.

In D15922#412417, @hfinkel wrote:

Upon reflection, cloning an outer loop is something we'll want to be able to do at some point. Why don't we just add the assertion for now, and then fix the function once we have a concrete use case?

Updating based on review comments

Updating based on review comments

[Cloning] Add assert to ensure no sub-loops for loop being cloned. Also rename
function to reflect its inability to clone sub-loops. (It can clone sub-loops,
but does not correctly update LoopInfo).

Hey sorry I'd forgotten about this patch. I'll work on it and resubmit in a day or two.

@joker.eph Thank you for the comments. I'll be happy to work on them (and take the patch to a good state for committing). However at the moment I'm looking for a broader approval that the code would be useful to the community and it would get in after its taken to a good state. I don't want to spend a lot of time reworking it and finally finding it to be not-useful and it not going in.

Thanks James for the review. Also you thank you for suggesting the original patch.