Page MenuHomePhabricator

[llvm] [Hexagon] Adding tests, TableGen entries, and code for emitting add opcode.
ClosedPublic

Authored by colinl on Oct 6 2014, 8:20 AM.

Details

Reviewers
sidneym
mcrosier
Summary

Updating .td files, updating some function names and adding functionality for emitting add opcode.

Diff Detail

Event Timeline

colinl updated this revision to Diff 14452.Oct 6 2014, 8:20 AM
colinl retitled this revision from to [llvm] [Hexagon] Adding tests, TableGen entries, and code for emitting add opcode..
colinl updated this object.
colinl edited the test plan for this revision. (Show Details)
colinl added reviewers: mcrosier, sidneym.
colinl set the repository for this revision to rL LLVM.
colinl added a subscriber: Unknown Object (MLST).
sidneym accepted this revision.Oct 8 2014, 7:19 AM
sidneym edited edge metadata.
This revision is now accepted and ready to land.Oct 8 2014, 7:19 AM
rafael added inline comments.
unittests/MC/Hexagon/HexagonMCCodeEmitterTest.cpp
57

Why can't you test this with llvm-mc? It seems a much better way of testing this then by manually creating instructions in a unit test.

colinl added inline comments.Oct 8 2014, 1:53 PM
unittests/MC/Hexagon/HexagonMCCodeEmitterTest.cpp
57

The plan was to get the integrated assember updated before putting in the assembler parser which is non-existent at the moment.

We thought it could also help narrowing down issues by testing the code emission in isolation.

What do you think?

sidneym added inline comments.Oct 9 2014, 7:37 AM
unittests/MC/Hexagon/HexagonMCCodeEmitterTest.cpp
57

That is right. The hexagon assembly language syntax is somewhat C-like and violates some of the assumptions the MC assembler makes.

We want to get the TD files updated with encoding bits and update the rest of MCTargetDesc so that object files can be directly emitted. Using this test framework allows us to add the encoding bits and have a directed test for each instruction.

That is right. The hexagon assembly language syntax is somewhat C-like and violates some of the assumptions the MC assembler makes.

We want to get the TD files updated with encoding bits and update the rest of MCTargetDesc so that object files can be directly emitted. Using this test framework allows us to add the encoding bits and have a directed test for each instruction.

Can you test the encoding with disassembler tests? Even if is printing
in a sudo-hexagon syntax for now that should be sufficient for
testing. I am afraid that using gtest for instruction encoding might
get even messier then the clang-format tests.

Cheers,
Rafael

In D5624#12, @rafael wrote:

That is right. The hexagon assembly language syntax is somewhat C-like and violates some of the assumptions the MC assembler makes.

We want to get the TD files updated with encoding bits and update the rest of MCTargetDesc so that object files can be directly emitted. Using this test framework allows us to add the encoding bits and have a directed test for each instruction.

Can you test the encoding with disassembler tests? Even if is printing
in a sudo-hexagon syntax for now that should be sufficient for
testing. I am afraid that using gtest for instruction encoding might
get even messier then the clang-format tests.

Cheers,
Rafael

Disassembler tests might be possible, I hadn't considered that before. We had thought the gtests would be sufficient since the inputs and outputs shouldn't vary as time goes on since the instructions and their encoding are tied to existing hardware.

What types of problems with clang-format tests are we looking to avoid? We have a fair number of instructions with running gtests we have pretty much prepped for upstreaming and haven't encountered any problems but we want to avoid something maybe we haven't thought of.

Disassembler tests might be possible, I hadn't considered that before. We had thought the gtests would be sufficient since the inputs and outputs shouldn't vary as time goes on since the instructions and their encoding are tied to existing hardware.

What types of problems with clang-format tests are we looking to avoid? We have a fair number of instructions with running gtests we have pretty much prepped for upstreaming and haven't encountered any problems but we want to avoid something maybe we haven't thought of.

With gtest you end up with a massive .cpp files which are hard to read
and hard to check against a native tool. IMHO it is also way too easy
to simply write gtest tests that just check that copy and paste is
working :-(

If you checkin a .o and test disassembly, things are much better IMHO:

  • You can check the .o agains a native tool or even the CPU.
  • You get an end to end test.
  • FileCheck error makes it very easy to see what instruction failed to

disassemble.

Cheers,
Rafael

In D5624#14, @rafael wrote:

Disassembler tests might be possible, I hadn't considered that before. We had thought the gtests would be sufficient since the inputs and outputs shouldn't vary as time goes on since the instructions and their encoding are tied to existing hardware.

What types of problems with clang-format tests are we looking to avoid? We have a fair number of instructions with running gtests we have pretty much prepped for upstreaming and haven't encountered any problems but we want to avoid something maybe we haven't thought of.

With gtest you end up with a massive .cpp files which are hard to read
and hard to check against a native tool. IMHO it is also way too easy
to simply write gtest tests that just check that copy and paste is
working :-(

If you checkin a .o and test disassembly, things are much better IMHO:

  • You can check the .o agains a native tool or even the CPU.
  • You get an end to end test.
  • FileCheck error makes it very easy to see what instruction failed to disassemble.

    Cheers, Rafael

Test quality is a good point. What we've been doing internally to keep the test quality high is using them as our source-of-truth for correct encoding. We thought unit-tests would be more of a library-centric approach rather than a toolchain-centric with files or calls out to other tools in the system.

Do you think we can push up a few more of these unit tests in the next week or so and see how it shapes up or is this more of a blocking issue? I think in the end you'll be happy with the quality.

Test quality is a good point. What we've been doing internally to keep the test quality high is using them as our source-of-truth for correct encoding. We thought unit-tests would be more of a library-centric approach rather than a toolchain-centric with files or calls out to other tools in the system.

Do you think we can push up a few more of these unit tests in the next week or so and see how it shapes up or is this more of a blocking issue? I think in the end you'll be happy with the quality.

I really don't want to have gtests for encoding in the long run. I
could see using them as a *short* term compromise, but It looks like
disassembler tests should do the job, why not use them?

Cheers,
Rafael

In D5624#16, @rafael wrote:

Test quality is a good point. What we've been doing internally to keep the test quality high is using them as our source-of-truth for correct encoding. We thought unit-tests would be more of a library-centric approach rather than a toolchain-centric with files or calls out to other tools in the system.

Do you think we can push up a few more of these unit tests in the next week or so and see how it shapes up or is this more of a blocking issue? I think in the end you'll be happy with the quality.

I really don't want to have gtests for encoding in the long run. I
could see using them as a *short* term compromise, but It looks like
disassembler tests should do the job, why not use them?

Cheers,
Rafael

With uploading .o files and disassembling them as a test we thought it would be difficult to document a process on how the public can regenerate the .o files if needed or add more in the future if we encounter a bug. We had thought adding unit tests would be more in line with the goals of being a library rather than a file-mover as with our implementation in a different toolchain.

One thing we could do to address the concern about a huge cpp file would be to break the files up in to logical units based on the instruction category in the documentation. We would have to make some changes internally but if that helps with organization we could make that change. Would you want us to do that when submitting follow up patches with tests?

With uploading .o files and disassembling them as a test we thought it would be difficult to document a process on how the public can regenerate the .o files if needed or add more in the future if we encounter a bug. We had thought adding unit tests would be more in line with the goals of being a library rather than a file-mover as with our implementation in a different toolchain.

One thing we could do to address the concern about a huge cpp file would be to break the files up in to logical units based on the instruction category in the documentation. We would have to make some changes internally but if that helps with organization we could make that change. Would you want us to do that when submitting follow up patches with tests?

Even if regenerating the .o files is not trivial before llvm-mc
getting support of the Hexagon assembly, it is probably still way more
maintainable to have

CHECK: some instruction in pseudo hexagon assembly syntax

then

59 {
60 llvm::raw_string_ostream stream(str);
61 llvm::HexagonMCInst inst(llvm::Hexagon::A2_add);
62 inst.addOperand(llvm::MCOperand::CreateReg(llvm::Hexagon::R17));
63 inst.addOperand(llvm::MCOperand::CreateReg(llvm::Hexagon::R21));
64 inst.addOperand(llvm::MCOperand::CreateReg(llvm::Hexagon::R31));
65 Emitter.Emitter->EncodeInstruction(inst, stream, EmptyFixups,
66 *Emitter.Subtarget);
67 }
68 ASSERT_EQ(4, str.size());
69 EXPECT_EQ(0x11, static_cast<uint8_t>(str[0]));
70 EXPECT_EQ(0xdf, static_cast<uint8_t>(str[1]));
71 EXPECT_EQ(0x15, static_cast<uint8_t>(str[2]));
72 EXPECT_EQ(0xf3, static_cast<uint8_t>(str[3]))

Using gtests is reasonable for things that are not otherwise exposed
in a tool. No tool in LLVM directly exposes a map, so a gtest is
perfect for testing DenseMap. In the case of encoding, it is exposed
via the assembler/disassembler.

Think of it as a special case of code refactoring. Instead of writing
a c++ test for every instruction, you can use a single program
(llvm-mc) and multiple inputs (instructions in a .o/.s).

Cheers,
Rafael

colinl updated this revision to Diff 14754.Oct 10 2014, 4:01 PM

I think I understand the reason for the request now. It sounds like you're saying the translation between assembly and machine code is most efficiently and compactly represented by assembling or disassembling and since we have a tool for that, it would be nice to use it. That logic makes sense.

I factored the unit test and it seems like they can be represented in a compact form which might satisfy the terseness request.

If this change is still rejected Sid is investigating disassembler patches though it make take a few to get all the required parts in place.

colinl updated this revision to Diff 15271.Oct 22 2014, 12:21 PM
colinl added a reviewer: mcrosier.

Adding tests using llvm-mc and removing unit tests.

colinl closed this revision.Oct 27 2014, 8:39 AM