Add ARC backend

[petecoup]

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.

[Eugene.Zelenko]

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]

llvm/lib/Target/ARC/ARCTargetTransformInfo.h

33

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

[petecoup]

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]

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]

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]

llvm/lib/Target/ARC/ARCExpandPseudos.cpp

98

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

[petecoup]

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]

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]

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]

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.

[petecoup]

Hello Matt, Eugene,

Any other comments here?
Thanks!

[Eugene.Zelenko]

In D36331#843388, @petecoup wrote:

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]

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]

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).

[kparzysz]

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]

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]

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).

[petecoup]

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.

[Eugene.Zelenko]

In D36331#846011, @petecoup wrote:

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]

In D36331#847065, @Eugene.Zelenko wrote:

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.

[petecoup]

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

[krytarowski]

In D36331#848714, @petecoup wrote:

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.

[petecoup]

Hello Kamil,

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

[• dberlin]

These copyright notices definitely should not be here :)

[petecoup]

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]

Can this review be closed?

[petecoup]

Yes, sorry for missing that.