[PATCH] [v5] BPF backend
Jim Grosbach
grosbach at apple.com
Wed Jan 14 19:11:53 PST 2015
Sent from my iPhone
> On Jan 13, 2015, at 11:11 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
>
> A bunch of miscellaneous code hygiene comments here. While these are fairly trivial, when a backend is coming into the project is the best time to do this kind of audit and get these things cleaned up. I've generally only commented once on issues even if they repeated. Please check for other instances of the coding standards stuff.
>
> The thing which is most important is the banner. The banner at the top of files should always be preserved.
>
> Note that I may have some suggestions here that are just out of the norm for backends as I'm not a backend expert. I've tried to check myself against them, and the backends I reference the most are the AArch64 backend and the X86 backend, but I may still just have some stuff wrong. Mostly, I'm focusing on general LLVM coding suggestions, and fairly high-level stuff. I generally trust that the logic and functionality is in good shape.
>
> Two high level things that I noticed going through this which you might want to address in follow-up patches (but that are too invasive IMO to try to do prior to it going in, and much lower risk of not getting cleaned up):
>
> 1. There seems to be inconsistency with whether single-statement if's or 'else's are braced or un-braced. My mild preference is to not use braces when all branches on the if/else chain are single statements, but I don't really care which as long as it is reasonably consistent within the backend.
> 2. There seem to be functions that would benefit from being split apart into static helper functions if just to comment the different sections and separate the logic better. This is mostly in the ISelLowering code.
>
> I've not been able to spot check the .td files very much as I'm not an expert there.
>
> I'd also love to get on MC expert to sign off on the assembler, disassembler, and general MC-layer sanity of the backend. In particular to make sure that all the important features, functionality, etc. are provided here so that this doesn't add yet another backend we have to update / migrate to newer tech.
>
I can look at the td files and general MC stuff, but it may take a while. A good review will take a bit on its own plus I'm pretty swamped per usual (I blame Evan ;) ). As long as the maintainers are willing to commit to making whatever clean ups and improvements are needed I don't mind doing that post-commit, however, especially if this goes in as experimental at first.
Jim
> I'll take another pass over the C++ code after you've done some of this cleanup.
>
>
> ================
> Comment at: include/llvm/IR/IntrinsicsBPF.td:1-6
> @@ +1,7 @@
> +//===- IntrinsicsBPF.td - Defines BPF intrinsics -----------*- tablegen -*-===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +
> ----------------
> Missing the bottom of the banner on this file. Please make it look exactly like the other intrinsics files.
>
> ================
> Comment at: include/llvm/IR/IntrinsicsBPF.td:17
> @@ +16,2 @@
> +}
> +
> ----------------
> I would trim trailing blank lines.
>
> ================
> Comment at: lib/Target/BPF/BPF.h:1-3
> @@ +1,4 @@
> +//===-- BPF.h - Top-level interface for BPF representation ----*- C++ -*-===//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +
> ----------------
> Please don't alter the banner at the top of each file. All of the files should have the standard banner.
>
> ================
> Comment at: lib/Target/BPF/BPF.h:5-6
> @@ +4,4 @@
> +
> +#ifndef TARGET_BPF_H
> +#define TARGET_BPF_H
> +
> ----------------
> Please follow the macro naming convention of the rest of LLVM, so for example LLVM_LIB_TARGET_AARCH64_AARCH64_H.
>
> ================
> Comment at: lib/Target/BPF/BPF.h:14-15
> @@ +13,4 @@
> +
> +/// createBPFISelDag - This pass converts a legalized DAG into a
> +/// BPF-specific DAG, ready for instruction scheduling.
> +FunctionPass *createBPFISelDag(BPFTargetMachine &TM);
> ----------------
> Use modern doxygen syntax as described here:
>
> http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
>
> ================
> Comment at: lib/Target/BPF/BPF.h:18
> @@ +17,3 @@
> +
> +extern Target TheBPFTarget;
> +}
> ----------------
> I prefer the X86 backend's pattern of declaring the extern targets explicitly in the few files they are needed rather than a header. I would declare them as locally as is reasonable.
>
> ================
> Comment at: lib/Target/BPF/BPF.td:8
> @@ +7,3 @@
> +
> +// BPF Subtarget features.
> +include "BPFRegisterInfo.td"
> ----------------
> These aren't subtarget features...
>
> ================
> Comment at: lib/Target/BPF/BPFISelLowering.cpp:83-84
> @@ +82,4 @@
> +
> +BPFTargetLowering::BPFTargetLowering(const TargetMachine &tm)
> + : TargetLowering(tm) {
> +
> ----------------
> s/tm/TM/
>
> ================
> Comment at: lib/Target/BPF/BPFISelLowering.cpp:183
> @@ +182,3 @@
> +
> +// Calling Convention Implementation
> +#include "BPFGenCallingConv.inc"
> ----------------
> Without the horizontal rule, the centered comment text really doesn't work.
>
> I don't care whether you use the horizontal rule style of commenting or not within the code itself (I don't care for horizontal rules, but I don't feel strongly about it), if you don't use it, don't center the text and do something else to set off the comment. Not sure that's even necessary here though given the include.
>
> ================
> Comment at: lib/Target/BPF/BPFISelLowering.cpp:198-199
> @@ +197,4 @@
> +
> + /// LowerCCCArguments - transform physical registers into virtual registers
> + /// and generate load operations for arguments places on the stack.
> + MachineFunction &MF = DAG.getMachineFunction();
> ----------------
> doxygen comment in the middle of the function?
>
> ================
> Comment at: lib/Target/BPF/BPFISelLowering.cpp:264-266
> @@ +263,5 @@
> + SDValue Callee = CLI.Callee;
> + bool &isTailCall = CLI.IsTailCall;
> + CallingConv::ID CallConv = CLI.CallConv;
> + bool isVarArg = CLI.IsVarArg;
> + MachineFunction &MF = DAG.getMachineFunction();
> ----------------
> Variable naming in LLVM is still 'ProudCamelCase', so these should be IsFooBar.
>
> ================
> Comment at: lib/Target/BPF/BPFISelLowering.cpp:280-281
> @@ +279,4 @@
> +
> + /// LowerCCCCallTo - functions arguments are copied from virtual regs to
> + /// (physical regs)/(stack frame), CALLSEQ_START and CALLSEQ_END are emitted.
> +
> ----------------
> Another doxygen comment in the function body...
>
> ================
> Comment at: lib/Target/BPF/BPFISelLowering.cpp:540
> @@ +539,3 @@
> + SelectionDAG &DAG) const {
> + SDLoc dl(Op);
> + const GlobalValue *GV = cast<GlobalAddressSDNode>(Op)->getGlobal();
> ----------------
> s/dl/DL/
>
> http://reviews.llvm.org/D6494
>
> EMAIL PREFERENCES
> http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list