Page MenuHomePhabricator

Add ARC backend
ClosedPublic

Authored by petecoup on Aug 4 2017, 11:07 AM.

Details

Summary

Add the ARC backend to lib/Target.

  • Targets ARC v2.
  • Includes a small functional subset of the 32-bit ISA, little endian.
  • Targets a fixed subset of the ARC v2 ISA.
  • Focuses on C99 support.
  • Implements Target registration, and a SelectionDAG based instruction selector.
  • Implements Register and the ISA specification for current subset.
  • Implements the Assembly printer and disassembler.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Please run Clang-tidy modernize checks over this code. Same for readability-container-size-empty, readability-redundant-control-flow, readability-redundant-member-init, readability-redundant-string-cstr, readability-redundant-string-init, readability-simplify-boolean-expr

llvm/lib/Target/ARC/ARC.h
34

Please add // LLVM_LIB_TARGET_ARC_ARC_H

llvm/lib/Target/ARC/ARCBranchFinalize.cpp
41

Please use default member initialization and nullptr.

llvm/lib/Target/ARC/ARCFrameLowering.cpp
102

Unnecessary empty line.

llvm/lib/Target/ARC/ARCFrameLowering.h
83

Please add // LLVM_LIB_TARGET_ARC_ARCFRAMELOWERING_H

llvm/lib/Target/ARC/ARCISelDAGToDAG.cpp
43

Unnecessary empty line.

llvm/lib/Target/ARC/ARCISelLowering.cpp
78

Unnecessary empty line.

563

Unnecessary empty line.

699

Unnecessary empty line.

llvm/lib/Target/ARC/ARCISelLowering.h
123

Please add // LLVM_LIB_TARGET_ARC_ARCISELLOWERING_H

llvm/lib/Target/ARC/ARCInstrInfo.h
39

Unnecessary line.

97

Please add // LLVM_LIB_TARGET_ARC_ARCINSTRINFO_H

llvm/lib/Target/ARC/ARCMCInstLower.h
14

Please separate with empty line.

43

Please add // LLVM_LIB_TARGET_ARC_ARCMCINSTLOWER_H

llvm/lib/Target/ARC/ARCMachineFunctionInfo.h
65

Please add // LLVM_LIB_TARGET_ARC_ARCMACHINEFUNCTIONINFO_H

llvm/lib/Target/ARC/ARCRegisterInfo.h
62

Please add // LLVM_LIB_TARGET_ARC_ARCREGISTERINFO_H

llvm/lib/Target/ARC/ARCSubtarget.cpp
32

InstrInfo() call is redundant.

llvm/lib/Target/ARC/ARCSubtarget.h
67

Please add // LLVM_LIB_TARGET_ARC_ARCSUBTARGET_H

llvm/lib/Target/ARC/ARCTargetMachine.h
53

Please add // LLVM_LIB_TARGET_ARC_ARCTARGETMACHINE_H

llvm/lib/Target/ARC/ARCTargetStreamer.h
25

Please add // LLVM_LIB_TARGET_ARC_ARCTARGETSTREAMER_H

llvm/lib/Target/ARC/ARCTargetTransformInfo.h
57

Please add // LLVM_LIB_TARGET_ARC_ARCTARGETTRANSFORMINFO_H

llvm/lib/Target/ARC/InstPrinter/ARCInstPrinter.h
20

Please separate with empty line.

48

Please add // LLVM_LIB_TARGET_ARC_INSTPRINTER_ARCINSTPRINTER_H

llvm/lib/Target/ARC/MCTargetDesc/ARCMCAsmInfo.h
33

Please add // LLVM_LIB_TARGET_ARC_MCTARGETDESC_ARCMCASMINFO_H

llvm/lib/Target/ARC/MCTargetDesc/ARCMCTargetDesc.cpp
75

{} -> = default;

llvm/lib/Target/ARC/MCTargetDesc/ARCMCTargetDesc.h
42

Please add // LLVM_LIB_TARGET_ARC_MCTARGETDESC_ARCMCTARGETDESC_H

Eugene.Zelenko added inline comments.Aug 4 2017, 3:46 PM
llvm/lib/Target/ARC/ARCTargetTransformInfo.h
33

Please use using instead of typedef. Same in other places.

petecoup updated this revision to Diff 109860.Aug 4 2017, 10:39 PM

Hello Eugene,
Thanks for the comments and suggestions. I think I've addressed you're comments, let me know if you find things I missed.

  • Run clang-tidy modernize, and fix resulting issues.
  • Add ending "#endif // THIS_FILE" to a number of files.
  • Remove a few needless extra lines, and a couple of required separating lines.
  • Change a couple typedef -> using.
Eugene.Zelenko added inline comments.Aug 5 2017, 2:15 PM
llvm/lib/Target/ARC/ARC.h
32

I think will be good idea to make namespace formatting uniform across files. It looks better when empty line separate namespace beginning and end. end namspace XYZ and end anonymous namespace are predominant closing comments in LLVM.

llvm/lib/Target/ARC/ARCBranchFinalize.cpp
2

Please make sure that such comment lines are 80 characters wide. It will be better to use only one dash (//===-) at beginning.

29

Unnecessary empty line.

41

Please add empty line before and after constructor body. Also after getPassName()

llvm/lib/Target/ARC/ARCISelDAGToDAG.cpp
40

Unnecessary empty comment.

arsenm added a subscriber: arsenm.Aug 7 2017, 9:23 AM
arsenm added inline comments.
llvm/lib/Target/ARC/ARCBranchFinalize.cpp
154–155

You should implement and use TII::getInstSizeInBytes for this instead

174

.str() is unnecessary

llvm/lib/Target/ARC/ARCExpandPseudos.cpp
80–81

You can avoid the ToRemove vector by directly using the iterator and incrementing it before you do the insert

98

.empty()

llvm/lib/Target/ARC/ARCISelLowering.cpp
188

You don't seem to implement allowsMisalignedMemoryAccesses which would get you a lot of this for free

256–257

I think you should remove any of these kinds of debug dumps. You already get these from the default selectionDAG debug output, so this is extra noise.

584

getStoreSize

Eugene.Zelenko added inline comments.Aug 7 2017, 10:44 AM
llvm/lib/Target/ARC/ARCExpandPseudos.cpp
98

This what Clang-tidy readability-container-size-empty should catch. I suggested such run before.

petecoup added inline comments.Aug 7 2017, 11:07 AM
llvm/lib/Target/ARC/ARCExpandPseudos.cpp
98

Hmm, that's odd, sorry about that. I will redo these checks, and address the other comments from here.

petecoup updated this revision to Diff 110257.Aug 8 2017, 12:56 PM

Hello Eugene, Matt,
Thanks again for taking a look. This should address all of the comments so far.

  • Change line-1 comments to be 80-characters, and unify format.
  • Add appropriate spacing around namespaces, and add '// end namespace XYZ' comments.
  • Fix some other spacing issues, and remove some empty comment lines.
  • Remove a couple of useless .str() calls.
  • Implement TII getInstSizeInBytes method. Use this method in ARCBranchFinalize pass.
  • Change Expand Pseudos to delete Instructions as encountered, rather than collect and delete at end.
  • Remove all target-specific Load/Store lowering: The target independent path works fine for what we have.
  • Add tests for unaligned load/store.
  • Remove noisy DEBUG output from ARCISelLowering.
  • Use getStoreSize where appropriate in ARCISelLowering.
  • Run missed clang-tidy checks from last time (fixes ARCTargetLowering::isLegalAddressingMode, static isBRccPseudo in ARCBranchFinalize).

Please let me know if I missed anything.

Eugene.Zelenko added inline comments.Aug 8 2017, 1:34 PM
llvm/lib/Target/ARC/ARC.td
2

One leading -, please.

llvm/lib/Target/ARC/ARCExpandPseudos.cpp
41

Please separate method from data with empty line.

petecoup updated this revision to Diff 110417.Aug 9 2017, 9:22 AM

Hello Eugene,

Thanks again for the comments here.

  • Change line-1 comment from '===--' to '===-' in a few .td files.
  • Add whitespace separating methods from data in a couple classes.

Hello Matt, Eugene,

Any other comments here?
Thanks!

Hello Matt, Eugene,

Any other comments here?
Thanks!

I think will be good idea to ask for review from maintainers of other backends. AArch64, ARM, Hexagon, AMDGPU have a lot of active contributors.

kparzysz added inline comments.
llvm/lib/Target/ARC/ARCFrameLowering.cpp
50

Variable names typically start with a capital letter. The 'dl' is kind of an exception (since DL usually refers to DataLayout), but then you have "amount" (starting with lowercase) and immediately after that "StackPtr", which is capitalized.

79

Function names, on the other names usually start with a lowercase.

115

The current guidelines are to omit the function names from the comments.

125

FYI, you can get the subtarget for your target via MF.getSubtarget<ARCSubtarget>(). This could avoid doing the static_cast, assuming that ARCSubtarget returns ARCInstrInfo.

157

You already have TII.

278

MBBI could be end(). It's better to use MBB.findDebugLoc(MBBI) instead.

285

Duplicate TII again.

llvm/lib/Target/ARC/ARCISelDAGToDAG.cpp
109

This is int32_t RHSC = RHS->getSExtValue(), the auto makes it look strange. In general we favor explicit type, unless the type is obvious and/or using auto saves space.

llvm/lib/Target/ARC/ARCISelLowering.cpp
342

This loop uses uppercase controlling variables, while the ones above use lowercase. Please make it consistent.

llvm/lib/Target/ARC/ARCRegisterInfo.cpp
41

This function is equivalent to ARC::GPR32RegClass.contains(Reg).

225

FYI, MI can be dumped directly to dbgs(), e.g. dbgs() << MI.

llvm/lib/Target/ARC/ARCSubtarget.h
59

This should return const ARCRegisterInfo*.

petecoup updated this revision to Diff 111538.Aug 17 2017, 11:32 AM

Hello Krzysztof,

Thanks for taking a look!
This new patch should address your comments here. Please let me know if I missed anything.

  • Rebase to trunk.
  • Change leading case on a number of functions/parameters/variables. Change debug location names to dl.
  • Remove some static_casts by using MF.getSubtarget<ARCSubtarget>() where appropriate.
  • Return ARCRegisterInfo* where appropriate.
  • Use MBB.findDebugLoc(MBBI) in a number of places where MBBI can be end().
  • Remove unnecessary TII variables.
  • Remove function names from a number of comments.
  • Remove other misleading/unnecessary comments, or commented out code.
  • Change int32_t vars to not be declared auto, and remove explicit casts.
  • Remove unnecessary isGPR32Register routine, and use ARC::GPR32RegClass.contains instead.
  • Use dbgs() << MI for MachineInstrs where appropriate.
  • Remove printMemOperand declaration (we use printMemOperandRI).

I found a few minor things, with those changes it looks good to go to me.

llvm/lib/Target/ARC/ARCFrameLowering.cpp
101

Please capitalize "allocate" and "scalarAlloc" below.

121

Could you spell this as AFI to make the capitalization consistent with other abbreviations?

llvm/lib/Target/ARC/Disassembler/ARCDisassembler.cpp
279

Please avoid "else" after "return".

petecoup updated this revision to Diff 111691.Aug 18 2017, 9:49 AM

Hello Krzysztof,

Thanks again!

  • Changed afi to AFI for ARCFunctionInfo variables.
  • Remove else after return in ARCDisassembler.cpp.
  • Add (void) to a couple variables that were only used in the with-asserts build to silence without-asserts build warnings (and change one to a better name while at it).
kparzysz accepted this revision.Aug 18 2017, 10:23 AM
This revision is now accepted and ready to land.Aug 18 2017, 10:23 AM

Please make sure that the copyright notices are ok with everyone. Other files don't have them and I don't know whether that works with the current license/legal status.

You'll probably need to modify some cmake file to add ARC to the list of experimental targets (I'm assuming you want to keep it as experimental for now).

Hello Krzysztof,

Thanks again!
Yes, this is going to be "experimental" for a while. I've enabled our builds with this via: -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD="ARC", so I don't think I need CMake changes here?
Yes, I'll make sure the copyright notices are kosher/resolved before submitting.

Hello Krzysztof,

Thanks again!
Yes, this is going to be "experimental" for a while. I've enabled our builds with this via: -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD="ARC", so I don't think I need CMake changes here?
Yes, I'll make sure the copyright notices are kosher/resolved before submitting.

Are experimental targets included in All targets? If not, do we have build bot which build experimental targets? Just to make sure that API of headers changes will not break them.

asb added a subscriber: asb.EditedAug 20 2017, 11:51 PM

Are experimental targets included in All targets? If not, do we have build bot which build experimental targets? Just to make sure that API of headers changes will not break them.

They are not. The convention is to set up a buildslave for the staging buildbot. This doesn't send emails upon failure, but allows the maintainer of an experimental target (and contributors) to check for build breakage.

Any comments or suggestions about Krzysztof's copyright notice question here? Are they OK in, or should the notices be taken out?

Any comments or suggestions about Krzysztof's copyright notice question here? Are they OK in, or should the notices be taken out?

I'm not the owner of LLVM, but they were the first thing I noticed in this code. I think they should be removed at least to be consistent with the majority of the code-base.

Hello Kamil,

Yes, that is a good point. I will remove them.
Thanks!

dberlin edited edge metadata.Aug 22 2017, 8:59 AM

These copyright notices definitely should not be here :)

petecoup updated this revision to Diff 112430.Aug 23 2017, 1:39 PM

Hello,

Thanks for the help/comments everyone. Is this all OK to submit?

  • Remove copyright notices
  • Add myself as the ARC backend owner in CODE_OWNERS.txt
kparzysz accepted this revision.Aug 23 2017, 1:42 PM

Can this review be closed?

petecoup closed this revision.Aug 31 2017, 12:46 PM

Yes, sorry for missing that.