Page MenuHomePhabricator

[TableGen][M68K] (Patch 1/8) Utilities for complex instruction addressing modes: CodeBeads and logical operand helper functions
Needs ReviewPublic

Authored by myhsu on Sep 27 2020, 4:22 PM.

Details

Summary

This patch adds two components: The CodeBeads TG backend and the support for InstrInfoEmitter TG backend to emit information about logical operands.

CodeBeads are annotations for instructions to express _non-trivial amount_ of addressing modes in an easier way. Many instructions of M68K, like many other CISC architectures, can be used with variety of addressing modes. For example, JSR (i.e. function call) can be used with four different addressing modes. Without this CodeBeads utility to explicitly embed these information into instructions' TG definitions, it would be more difficult for MC to encode the instructions (e.g. need to write some sort of pattern matching code to figure out the addressing mode).

Another special property of CISC ISA is that a single instruction operand (called "logical operand" here) might consist of multiple llvm::MachineOperands. Thus this patch enables IntrInfoEmitter TG backend to (optionally) generate helper functions for logical operands.
More specifically, the llvm::<target NS>::getLogicalOperandSize to get the number of llvm::MachineOperand on a specific logical operand; llvm::<target NS>::getLogicalOperandIdx to get the llvm::MachineOperand index for a specific logical operand.

Diff Detail

Event Timeline

myhsu created this revision.Sep 27 2020, 4:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2020, 4:22 PM
myhsu requested review of this revision.Sep 27 2020, 4:22 PM

Everything looks good except for the Clang tidies that need attention. A couple of them pertain to the skeleton backend file, so I will fix those in that file, too.

Let's wait a couple of days to see if anyone else has comments.

hansec removed a reviewer: hansec.Sep 28 2020, 9:57 AM
jrtc27 requested changes to this revision.Sep 29 2020, 7:01 PM
jrtc27 added a subscriber: jrtc27.
jrtc27 added inline comments.
llvm/utils/TableGen/CodeBeadsGen.cpp
10

This needs to be filled in before landing.

llvm/utils/TableGen/TableGen.cpp
28

I don't understand what order these are in, so why here rather than at the end of the enum?

This revision now requires changes to proceed.Sep 29 2020, 7:01 PM
myhsu updated this revision to Diff 295461.Sep 30 2020, 9:56 PM
  • Fix formatting issues
  • Add documentation for CodeBeads in the header comment

There are plenty of lint checks that need addressing. Some are false positives and those are obvious cases, but the rest should be followed.

You must run clang-format on new files (entirely) and on localised changes of existing files (many editors have the option to select lines and format).

llvm/utils/TableGen/CodeBeadsGen.cpp
6

The license has changed to the Apache license (+LLVM exception).

See LICENSE.TXT.

You'll need to change that in all your files.

rengolin added a subscriber: craig.topper.

Adding @craig.topper as the owner of the x86 back-end, to check the implementation of the CodeBeads concept. I have mainly worked with RISC back-ends, so it's not my area. Craig, please feel free to add whoever you think is best to review this.

myhsu added a comment.Oct 1 2020, 4:08 PM

There are plenty of lint checks that need addressing. Some are false positives and those are obvious cases, but the rest should be followed.

You must run clang-format on new files (entirely) and on localised changes of existing files (many editors have the option to select lines and format).

Interesting...the patch I updated yesterday had gone through clang-format. And I didn't see any lint warning here on Phabricator. Maybe I should run again.

Don't forget to change to the current license.

myhsu updated this revision to Diff 295702.Oct 1 2020, 5:21 PM

Update license

myhsu marked 3 inline comments as done.Oct 1 2020, 5:21 PM

Interesting...the patch I updated yesterday had gone through clang-format. And I didn't see any lint warning here on Phabricator. Maybe I should run again.

Did you check that you were using the latest llvm configuration?

myhsu updated this revision to Diff 296009.Oct 3 2020, 4:42 PM

Decouple getGenInstrBeads from <Target>MCCodeEmitter and put it as a static function in llvm::<Target namespace> with new name getMCInstrBeads

myhsu added a comment.Oct 3 2020, 4:45 PM

Interesting...the patch I updated yesterday had gone through clang-format. And I didn't see any lint warning here on Phabricator. Maybe I should run again.

Did you check that you were using the latest llvm configuration?

I made sure clang-format was fetching the the right config file. Would you mind pointing out some of the places that you think ill-format? thank you!

craig.topper added inline comments.Oct 6 2020, 8:09 PM
llvm/utils/TableGen/CMakeLists.txt
59

This list was nearly in alphabetical order. Can you put this file in the correct place.

llvm/utils/TableGen/CodeBeadsGen.cpp
51

Where do these numbers come from? Are they specific to 68K?

86

Remove commented out code?

93

You can use auto here

98

I think you can use Twine(i) here instead of std::to_string(i).

llvm/utils/TableGen/InstrInfoEmitter.cpp
466

std::max?

470

Is the find and if necessary? Won't insert avoid overwriting value if its already in the map?

474

I think you can use (Namespace + "::" + Inst->TheDef->getName()).str(). That will create a Twine for the pieces and the convert it to a std::string at the end. This should be slightly more efficient than concatenating std::strings.

497

Probably don't need to bother avoiding trailing commas in the generated file. The C++ parsing rules allow them

518

I think we should just use a range for loop here. llvm::for_each is discouraged here https://llvm.org/docs/CodingStandards.html#use-range-based-for-loops-wherever-possible

561

Same comment as earlier about using a Twine here.

567

std::max?

570

I think insert is sufficient.

618

range for

myhsu updated this revision to Diff 297344.Oct 9 2020, 3:38 PM
myhsu marked 13 inline comments as done.

Addressed feedbacks, mostly formatting issues and minor coding improvements

llvm/utils/TableGen/CodeBeadsGen.cpp
51

no really...this number depends on maximum bit length among all code beads. As the TODO comment on line 55 suggested we should have a way to evaluate this dynamically. Also the for loop on line 84 need this number to reverse the byte order

Paul-C-Anagnostopoulos resigned from this revision.Oct 13 2020, 3:56 PM

A minor suggestion: Use the new true/false literals in the TableGen files. I believe it makes the code easier to read.

myhsu added a reviewer: RKSimon.
RKSimon added inline comments.Mon, Nov 16, 11:01 AM
llvm/utils/TableGen/CodeBeadsGen.cpp
51

Please add a comment explaining these magic numbers.

llvm/utils/TableGen/InstrInfoEmitter.cpp
489

const auto &Row

507

const auto &Instrs

581

const auto &Row

605

const auto &Insts

llvm/include/llvm/Target/Target.td
638

Before you push this, Target.td will use 'true' and 'false'.

llvm/utils/TableGen/CodeBeadsGen.cpp
34

It's conventional to call the output stream OS.

myhsu marked 4 inline comments as done.Tue, Nov 17, 3:47 PM
myhsu added inline comments.
llvm/utils/TableGen/CodeBeadsGen.cpp
51

Eventually I decided to just fix that TODO so the next revision will not have that magic number anymore

myhsu updated this revision to Diff 305917.EditedTue, Nov 17, 3:57 PM
myhsu marked 5 inline comments as done.
  • Rebase to upstream master
  • Addressed feedbacks
  • Update to use the latest TG syntax