Page MenuHomePhabricator

Adding skeleton for unit testing Hexagon Code Emission
ClosedPublic

Authored by colinl on Sep 29 2014, 8:59 AM.

Details

Summary

Adding and modifying CMakeLists.txt files to run unit tests under unittests/Target/* if the directory exists.
Adding basic unit test to check that code emitter object can be retrieved.

Diff Detail

Event Timeline

colinl updated this revision to Diff 14167.Sep 29 2014, 8:59 AM
colinl retitled this revision from to Adding skeleton for unit testing Hexagon Code Emission.
colinl updated this object.
colinl edited the test plan for this revision. (Show Details)
colinl set the repository for this revision to rL LLVM.
colinl added a subscriber: Unknown Object (MLST).
mcrosier added inline comments.
lib/Target/Hexagon/MCTargetDesc/HexagonMCCodeEmitter.cpp
35

Please add a period.

38

No need for curly brackets, if the body is a single statement.

39

Where are these magic numbers coming from?

74

Reduce indent and remove curly brackets.

if (MO.isReg())

return MCT.getRegisterInfo()->getEncodingValue(MO.getReg());

if (MO.isImm())

return static_cast<unsigned>(MO.getImm());
unittests/Target/CMakeLists.txt
6 ↗(On Diff #14167)

Please add newline.

unittests/Target/Hexagon/CMakeLists.txt
11 ↗(On Diff #14167)

Please add newline.

unittests/Target/Hexagon/HexagonMCCodeEmitterTest.cpp
54 ↗(On Diff #14167)

Please add newline.

colinl updated this revision to Diff 14185.Sep 29 2014, 1:08 PM

Applying formatting updates.
Adding enumeration to explain bitfield and constant for bitfield offsets.

Once the comments are address, I believe this is in good enough shape. Please upload a revised patch and I'll approve.

lib/Target/Hexagon/MCTargetDesc/HexagonMCCodeEmitter.cpp
11

IIRC, I believe this should be added after the #includes.

42

How about something like this?

ParseField Field = HMI.isPacketEnd() ? ParseField::end : ParseField::last0;
return static_cast<uint32_t>(Field) << ParseFieldOffset;

unittests/CMakeLists.txt
26 ↗(On Diff #14185)

Would this be better in

unittests/MC/Target

or

unittests/MC/Hexagon (I prefer)

?

The latter suggestion is more inline with how test/MC/Hexagon and test/CodeGen/Hexagon are laid out.

unittests/Target/Hexagon/HexagonMCCodeEmitterTest.cpp
23 ↗(On Diff #14185)

assert(Target && "Expected to find a target.");

24 ↗(On Diff #14185)

Please use the
assert(cond && "msg");
style.

Many more instances below.

Sorry for the multiple reviews.. Please address these as well.

lib/Target/Hexagon/MCTargetDesc/HexagonMCCodeEmitter.cpp
65

How about just:

uint64_t Binary = getBinaryCodeForInstr(HMB, Fixups, STI) | getPacketBits(HMB);

68

assert(cond && "msg");

colinl updated this revision to Diff 14228.Sep 30 2014, 8:38 AM
mcrosier accepted this revision.Sep 30 2014, 9:05 AM
mcrosier added a reviewer: mcrosier.
This revision is now accepted and ready to land.Sep 30 2014, 9:05 AM
sidneym edited edge metadata.Oct 3 2014, 6:11 AM

Looks good and I will submit this for you today. Chris Lattner can grant you check-in permission.

emaste added a subscriber: emaste.Oct 3 2014, 6:59 AM
emaste added inline comments.
lib/Target/Hexagon/CMakeLists.txt
5

Needs to be added to Makefile too?

colinl closed this revision.Oct 6 2014, 8:10 AM

Patch applied.