Page MenuHomePhabricator

Add build_fence to OCaml bindings
AcceptedPublic

Authored by frabert on Apr 30 2021, 8:10 AM.

Details

Summary

Adds support for calling LLVMBuildFence from the OCaml bindings

Diff Detail

Event Timeline

frabert created this revision.Apr 30 2021, 8:10 AM
frabert requested review of this revision.Apr 30 2021, 8:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2021, 8:10 AM
frabert updated this revision to Diff 341951.Apr 30 2021, 9:56 AM

Fix formatting issues

frabert updated this revision to Diff 341960.Apr 30 2021, 10:02 AM

Squash changes

frabert updated this revision to Diff 342163.May 1 2021, 10:38 AM
jberdine requested changes to this revision.May 1 2021, 1:12 PM
jberdine added a subscriber: jberdine.

Looks good apart from a typo, and a bit more documentation.

llvm/bindings/ocaml/llvm/llvm.ml
1351
llvm/bindings/ocaml/llvm/llvm.mli
2600–2604

It would be good to also document the [ord], [st], and [name] arguments. See e.g. the docstring for [build_atomicrmw] for how it describes the first two.

This revision now requires changes to proceed.May 1 2021, 1:12 PM
frabert updated this revision to Diff 342173.May 1 2021, 1:33 PM

Address review

frabert marked an inline comment as done.May 1 2021, 1:38 PM
frabert added inline comments.
llvm/bindings/ocaml/llvm/llvm.mli
2600–2604

I have addressed the issue concerning the first two arguments, they now have documentation comparable to that of [build_atomicrmw].

I am not sure how to approach [name], though: I couldn't find from a first glance any [build_*] function which actually documents its use, leaving it implicitly assumed that it will be the name of the result.

jberdine added inline comments.May 1 2021, 2:02 PM
llvm/bindings/ocaml/llvm/llvm.mli
2600–2604

Would it make sense to do the same with name as in e.g. build_freeze just above?

frabert updated this revision to Diff 342179.May 1 2021, 2:17 PM
frabert added inline comments.May 1 2021, 2:19 PM
llvm/bindings/ocaml/llvm/llvm.mli
2600–2604

I tried merging the two approaches. I have not found a way to succintly express the singlethread flag in the IR syntax, so I left that particular parameter out of the generated instruction, only mentioning in the doc comment.

frabert marked 2 inline comments as done.May 4 2021, 2:02 AM

Thanks, the docs look good.

I'm sorry to give feedback piecemeal, but it would also be good to add a basic test to llvm/test/Bindings/OCaml/core.ml.

frabert updated this revision to Diff 342713.May 4 2021, 6:12 AM

Add tests

jberdine accepted this revision.May 4 2021, 6:14 AM

Thanks! LGTM

This revision is now accepted and ready to land.May 4 2021, 6:14 AM

I'm sorry to give feedback piecemeal, but it would also be good to add a basic test to llvm/test/Bindings/OCaml/core.ml.

No problem, only issue is I'm not really set up to run the tests right now, so for the time being I can only do stuff by copying. The automated checks are K.O. too, it seems, unfortunately.

I will try the tests locally

jberdine requested changes to this revision.May 4 2021, 7:15 AM

There is an issue. The fence instruction always returns type void. An SSA name is not permitted for values of type void. The test complains:

> ninja bindings/ocaml/all check-llvm-bindings-ocaml
[17/18] Running lit suite /Users/jjb/src/llvm-project/llvm/test/Bindings/OCaml
FAIL: LLVM :: Bindings/OCaml/core.ml (1 of 16)
******************** TEST 'LLVM :: Bindings/OCaml/core.ml' FAILED ********************
Script:
--
: 'RUN: at line 1';   rm -rf /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp && mkdir -p /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp && cp /Users/jjb/src/llvm-project/llvm/test/Bindings/OCaml/core.ml /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/core.ml && cp /Users/jjb/src/llvm-project/llvm/test/Bindings/OCaml/Utils/Testsuite.ml /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/Testsuite.ml
: 'RUN: at line 2';   /Users/jjb/.opam/sledge/bin/ocamlfind ocamlc -cclib -L/Users/jjb/src/llvm-project/_build/test/../lib  -g -w +A -package llvm.analysis -package llvm.bitwriter -I /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/ -linkpkg /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/Testsuite.ml /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/core.ml -o /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/executable
: 'RUN: at line 3';   /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/executable /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/bitcode.bc
: 'RUN: at line 4';   /Users/jjb/.opam/sledge/bin/ocamlfind ocamlopt -cclib -L/Users/jjb/src/llvm-project/_build/test/../lib -cclib -Wl,-rpath,/Users/jjb/src/llvm-project/_build/test/../lib  -g -w +A -package llvm.analysis -package llvm.bitwriter -I /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/ -linkpkg /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/Testsuite.ml /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/core.ml -o /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/executable
: 'RUN: at line 5';   /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/executable /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/bitcode.bc
: 'RUN: at line 6';   /Users/jjb/src/llvm-project/_build/bin/llvm-dis < /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/bitcode.bc > /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/dis.ll
: 'RUN: at line 7';   /Users/jjb/src/llvm-project/_build/bin/FileCheck /Users/jjb/src/llvm-project/llvm/test/Bindings/OCaml/core.ml < /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/dis.ll
: 'RUN: at line 9';   /Users/jjb/src/llvm-project/_build/bin/FileCheck -check-prefix=CHECK-NOWHERE /Users/jjb/src/llvm-project/llvm/test/Bindings/OCaml/core.ml < /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/dis.ll
--
Exit Code: 134

Command Output (stderr):
--
Assertion failed: (!getType()->isVoidTy() && "Cannot assign a name to void values!"), function setNameImpl, file /Users/jjb/src/llvm-project/llvm/lib/IR/Value.cpp, line 331.
/Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.script: line 8: 36476 Abort trap: 6           /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/executable /Users/jjb/src/llvm-project/_build/test/Bindings/OCaml/Output/core.ml.tmp/bitcode.bc

--

********************
********************
Failed Tests (1):
  LLVM :: Bindings/OCaml/core.ml


Testing Time: 11.17s
  Passed: 15
  Failed:  1
llvm/test/Bindings/OCaml/core.ml
1177–1182
This revision now requires changes to proceed.May 4 2021, 7:15 AM

Makes sense, but I don't really understand what purpose the string parameter serves, then. Is it supposed to always be the empty string?

frabert updated this revision to Diff 342796.May 4 2021, 10:30 AM
frabert marked an inline comment as done.

I did a bit of hunting. The name string must be empty or else LLVMBuildFence will assert. It seems to me that

FenceInst *CreateFence(AtomicOrdering Ordering,
                       SyncScope::ID SSID = SyncScope::System,
                       const Twine &Name = "") {
  return Insert(new FenceInst(Context, Ordering, SSID), Name);
}

in IRBuilder.h should be

FenceInst *CreateFence(AtomicOrdering Ordering,
                       SyncScope::ID SSID = SyncScope::System) {
  return Insert(new FenceInst(Context, Ordering, SSID));
}

and LLVMBuildFence should not take Name as an argument.

A solution that is local to the OCaml bindings would be to just always pass the empty string, and the underlying issue in IRBuilder can be addressed separately.

frabert updated this revision to Diff 342953.May 4 2021, 11:29 PM

Remove [name] parameter

jberdine accepted this revision.May 5 2021, 2:03 PM

LGTM now, thanks! FTR local testing yields:

❯ ninja bindings/ocaml/all check-llvm-bindings-ocaml
[2598/2599] Running lit suite /Users/jjb/src/llvm-project/llvm/test/Bindings/OCaml

Testing Time: 11.40s
  Passed: 16
This revision is now accepted and ready to land.May 5 2021, 2:03 PM