[PATCH] [v5] BPF backend
Alexei Starovoitov
alexei.starovoitov at gmail.com
Wed Jan 14 00:13:02 PST 2015
> There seems to be inconsistency with whether single-statement if's or 'else's are braced or un-braced
in general I've tried not to waste extra line on a last brace for single line statements, but looks like
I've missed few cases.. will double check.
> The thing which is most important is the banner. The banner at the top of files should always be preserved.
the banner felt way too verbose. I kept copyright and general text, but removed ----- lines.
Sure. Will restore verbosity :)
Thank you for the review!
================
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.
+
----------------
chandlerc wrote:
> Missing the bottom of the banner on this file. Please make it look exactly like the other intrinsics files.
you mean something like:
//===----------------------------------------------------------------------===//
//
// This file defines all of the X86-specific intrinsics.
//
//===----------------------------------------------------------------------===//
I felt that all these unnecessary verbose lines don't provide any
information into the logic of the file. Only increase line count...
Sure... will restore verbosity here and everywhere.
================
Comment at: include/llvm/IR/IntrinsicsBPF.td:17
@@ +16,2 @@
+}
+
----------------
chandlerc wrote:
> I would trim trailing blank lines.
ok
================
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.
+
----------------
chandlerc wrote:
> Please don't alter the banner at the top of each file. All of the files should have the standard banner.
sure. just tried to reduce verbosity...
================
Comment at: lib/Target/BPF/BPF.h:5-6
@@ +4,4 @@
+
+#ifndef TARGET_BPF_H
+#define TARGET_BPF_H
+
----------------
chandlerc wrote:
> Please follow the macro naming convention of the rest of LLVM, so for example LLVM_LIB_TARGET_AARCH64_AARCH64_H.
ok
================
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);
----------------
chandlerc wrote:
> Use modern doxygen syntax as described here:
>
> http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
was a copy-paste from MSP430... Sure. will fix in other places. here just going to remove it to match style of other backends.
================
Comment at: lib/Target/BPF/BPF.h:18
@@ +17,3 @@
+
+extern Target TheBPFTarget;
+}
----------------
chandlerc wrote:
> 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.
that was the style of R600 and NVPTX, Yes, makes sense to reduce the scope.
================
Comment at: lib/Target/BPF/BPF.td:8
@@ +7,3 @@
+
+// BPF Subtarget features.
+include "BPFRegisterInfo.td"
----------------
chandlerc wrote:
> These aren't subtarget features...
copy paste left over. good catch. thanks
================
Comment at: lib/Target/BPF/BPFISelLowering.cpp:83-84
@@ +82,4 @@
+
+BPFTargetLowering::BPFTargetLowering(const TargetMachine &tm)
+ : TargetLowering(tm) {
+
----------------
chandlerc wrote:
> s/tm/TM/
hexagon and systemz are using lower case... sure. will upper case it, since that's a preferred way.
================
Comment at: lib/Target/BPF/BPFISelLowering.cpp:183
@@ +182,3 @@
+
+// Calling Convention Implementation
+#include "BPFGenCallingConv.inc"
----------------
chandlerc wrote:
> 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.
I wish I knew why arm, aarch64 and others are doing this way...
the only thing I did is dropped:
//===----------------------------------------------------------------------===//
around it that looks way too verbose to me.
Agree. It looks out of place with the rest. Will simplify.
================
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();
----------------
chandlerc wrote:
> doxygen comment in the middle of the function?
in the earlier versions it was a helper function with single caller. I manually inlined it and kept the comment.
I guess that's what you were hinting about splitting them up into helpers. Will uninline.
================
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();
----------------
chandlerc wrote:
> Variable naming in LLVM is still 'ProudCamelCase', so these should be IsFooBar.
Actually that part of coding style is the most confusing:
$ git grep isVarArg|wc -l
476
$ git grep IsVarArg|wc -l
126
I see the same in x86, sparc and other backends....
so I think I picked the right one...
================
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.
+
----------------
chandlerc wrote:
> Another doxygen comment in the function body...
will uninline into helper
================
Comment at: lib/Target/BPF/BPFISelLowering.cpp:540
@@ +539,3 @@
+ SelectionDAG &DAG) const {
+ SDLoc dl(Op);
+ const GlobalValue *GV = cast<GlobalAddressSDNode>(Op)->getGlobal();
----------------
chandlerc wrote:
> s/dl/DL/
I don't think there is a consistency:
$ git grep 'SDLoc dl('|wc -l
499
$ git grep 'SDLoc DL('|wc -l
326
I think I picked the right one...
http://reviews.llvm.org/D6494
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list