Page MenuHomePhabricator

[llvm-ocaml] Revise OCaml opaque pointer bindings to match rest of API
AbandonedPublic

Authored by alan on Oct 7 2022, 4:18 PM.

Details

Diff Detail

Unit TestsFailed

TimeTest
60,050 msx64 debian > MLIR.Examples/standalone::test.toy
Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir -DLLVM_USE_LINKER=lld -DPython3_EXECUTABLE="/usr/bin/python3.9"

Event Timeline

alan created this revision.Oct 7 2022, 4:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2022, 4:18 PM
alan requested review of this revision.Oct 7 2022, 4:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2022, 4:18 PM
alan added a comment.EditedOct 7 2022, 4:47 PM

Per the discussion on https://discourse.llvm.org/t/i-accidentally-created-a-naming-inconsistency-in-the-ocaml-api/65715, I renamed the pointer_type_in_context function I previously added. Because this function takes an address space as an argument, I decided to rename it to qualified_pointer_type2, rather than pointer_type2, to match the existing qualified_pointer_type function, which also takes an address space as an argument. Then, I added another function, pointer_type2, which uses the opaque pointer API and does not take an address space as an argument, to correspond with the pointer_type function.

I also changed as to a in the documentation because as is an OCaml keyword and therefore not a valid variable name. (The occurrence of as in the documentation comment was colored like a keyword in my editor, so it stood out to me.)

alan edited reviewers, added: aeubanks; removed: nikic.Oct 7 2022, 4:52 PM
alan updated this revision to Diff 466229.Oct 7 2022, 5:46 PM

Make doc comment of pointer_type2 refer to getUnqual, not get

alan added a comment.Oct 7 2022, 6:11 PM

Also, I chose not to use the word "opaque" in the docs because once typed pointers are phased out, *all* pointers will be opaque pointers.

alan added a comment.EditedOct 7 2022, 6:46 PM

According to https://llvm.org/docs/OpaquePointers.html#version-support, LLVM 16 will not support typed pointers. So, if this patch is not going into an LLVM 15 minor or patch release, could one totally remove the old pointer_type and qualified_pointer_type functions and give their names to pointer_type2 and qualified_pointer_type2? However, that involves rewriting the tests, which use the old functions, and I'm not an expert on what all the tests are supposed to check...

alan added a reviewer: nikic.Oct 8 2022, 7:06 PM

I now made https://reviews.llvm.org/D135524, which is an alternative to this patch. For context, see https://discourse.llvm.org/t/i-accidentally-created-a-naming-inconsistency-in-the-ocaml-api/65715. I intend for either one or the other to be merged depending on which my reviewers think is more appropriate.