Page MenuHomePhabricator

[C API] Add test for D25259 and new LLVMIsExact function.
Needs ReviewPublic

Authored by deadalnix on Oct 4 2016, 10:19 PM.

Details

Summary

The LLVMIsExact function was added to make this test possible, but it is
generally useful.

Diff Detail

Event Timeline

mjacob updated this revision to Diff 73590.Oct 4 2016, 10:19 PM
mjacob retitled this revision from to [C API] Add test for D25259 and new LLVMIsExact function..
mjacob updated this object.
mjacob added reviewers: mehdi_amini, deadalnix.
mjacob added a subscriber: llvm-commits.
deadalnix added inline comments.Oct 5 2016, 12:11 AM
include/llvm-c/Core.h
3289–3290

Don't put that in the middle of the builder mapping.

tools/llvm-c-test/echo.cpp
503–506

This begs the question: should this have been done via a SetExact function ?

mjacob abandoned this revision.Oct 5 2016, 7:54 AM

I wasn't very happy with the approach already when submitting the patch.

  • It adds a new C API function (LLVMIsExact) which I don't really care about other than for writing this test.
  • It's asymmetric (e.g. missing something like LLVMSetIsExact). @deadalnix pointed this out as well.
  • LLVMConstExactUDiv is still untested.

I think we need a different approach for testing the C API. Something like calling a bunch of C API functions, dumping the module and checking it with FileCheck.

mehdi_amini edited edge metadata.Oct 5 2016, 8:38 AM
  • It adds a new C API function (LLVMIsExact) which I don't really care about other than for writing this test.
  • It's asymmetric (e.g. missing something like LLVMSetIsExact). @deadalnix pointed this out as well.

The general question I'd have would be: isn't the ability to round-trip something that is "useful" in general with the C API?
If yes then it seems that the test helped identified the missing LLVMIsExact, and it is legit.
Otherwise yeah it isn't great to add API just for being able to fit the testing infrastructure and fit other APIs.

  • LLVMConstExactUDiv is still untested.

Can't you round-trip the following?

@foo = external global i8
define i64 @test() {
  ret i64 udiv exact (i64 ptrtoint (i8* @foo to i64), i64 10)
}

I think we need a different approach for testing the C API. Something like calling a bunch of C API functions, dumping the module and checking it with FileCheck.

I'd be fine with that, but do we have the infrastructure for it?

mjacob added a comment.Oct 5 2016, 9:11 AM
  • It adds a new C API function (LLVMIsExact) which I don't really care about other than for writing this test.
  • It's asymmetric (e.g. missing something like LLVMSetIsExact). @deadalnix pointed this out as well.

The general question I'd have would be: isn't the ability to round-trip something that is "useful" in general with the C API?
If yes then it seems that the test helped identified the missing LLVMIsExact, and it is legit.
Otherwise yeah it isn't great to add API just for being able to fit the testing infrastructure and fit other APIs.

Yes, being able to roundtrip is useful. The obvious roundtrip would be through LLVMIsExact and LLVMSetIsExact. LLVMSetIsExact and LLVMBuildExactUDiv is less obvious and is what I meant with "asymmetric".

Sure, if the point was to add LLVMIsExact and LLVMSetIsExact, the echo test would probably be the perfect place to test this.

  • LLVMConstExactUDiv is still untested.

Can't you round-trip the following?

@foo = external global i8
define i64 @test() {
  ret i64 udiv exact (i64 ptrtoint (i8* @foo to i64), i64 10)
}

Yes, something like that could be added if we decide that the echo test is the right place to add it to.

I think we need a different approach for testing the C API. Something like calling a bunch of C API functions, dumping the module and checking it with FileCheck.

I'd be fine with that, but do we have the infrastructure for it?

I tried this and I think it should work with the current infrastructure. We'd have a file tools/llvm-c-test/test_ir.c with a lot of code like this:

// CHECK-NEXT: %add = add i64 %x, %y
LLVMBuildAdd(Builder, X, Y, "add");

and a file test/Bindings/llvm-c/test_ir.test with only the following content:

RUN: llvm-c-test --test-ir 2>&1 | FileCheck %S/../../../tools/llvm-c-test/test_ir.c

(or however this can be expressed cleaner).

  • It adds a new C API function (LLVMIsExact) which I don't really care about other than for writing this test.
  • It's asymmetric (e.g. missing something like LLVMSetIsExact). @deadalnix pointed this out as well.

The general question I'd have would be: isn't the ability to round-trip something that is "useful" in general with the C API?
If yes then it seems that the test helped identified the missing LLVMIsExact, and it is legit.
Otherwise yeah it isn't great to add API just for being able to fit the testing infrastructure and fit other APIs.

Yes, being able to roundtrip is useful. The obvious roundtrip would be through LLVMIsExact and LLVMSetIsExact. LLVMSetIsExact and LLVMBuildExactUDiv is less obvious and is what I meant with "asymmetric".

I understood the "asymmetric issue", unfortunately that's something that comes up at the C++ API level as well, so it is not really a C API issue here.

Sure, if the point was to add LLVMIsExact and LLVMSetIsExact, the echo test would probably be the perfect place to test this.

My point was rather: the fact that the test is about round-trip pointed a "hole" in the API with the lack of "setIsExact".

deadalnix updated this revision to Diff 85646.Jan 24 2017, 3:37 PM
deadalnix edited edge metadata.

Pinging this. I think this is fine and should proceed.

deadalnix commandeered this revision.Jun 4 2018, 4:26 PM
deadalnix edited reviewers, added: mjacob; removed: deadalnix.
deadalnix added a reviewer: whitequark.
mjacob added a comment.Jun 4 2018, 7:03 PM

I'm really sorry to not have finished this, despite telling you that I'll do it.

The one remaining issue: where do we move the LLVMIsExact to? After LLVMBuildExactSDiv? Something similar was done with LLVMIsAtomicSingleThread, which is right after the LLVMBuildAtomicCmpXchg that creates atomic instructions.

I think we need to add LLVMSetExact as well.