[PATCH] Initial contribution of llgo, a Go frontend

Peter Collingbourne peter at pcc.me.uk
Fri Nov 21 17:04:06 PST 2014


I decided to address the things that were easy to address. We can probably address the other items later.

================
Comment at: tools/llgo/CMakeLists.txt:44
@@ +43,3 @@
+  DEPENDS libgo ${CMAKE_BINARY_DIR}/bin/llgo${CMAKE_EXECUTABLE_SUFFIX}
+  GOFLAGS "cc=${CMAKE_BINARY_DIR}/bin/clang"
+          "cxx=${CMAKE_BINARY_DIR}/bin/clang++"
----------------
ruiu wrote:
> Why do you use locally built clang to build go? Can't it compile with system's C/C++ compiler?
It can (this is how the stage1 compiler is built), but these two targets (the stage2 and stage3 compilers) are used for bootstrapping, so they use previous compiler stages.

================
Comment at: tools/llgo/README.TXT:17-18
@@ +16,4 @@
+
+llgo is currently only supported on the x86-64 Linux platform. Contributions
+that add support for other platforms are welcome.
+
----------------
emaste wrote:
> Could you add a brief high-level description of platform-specific code required? I.e., where should someone who wants to support other platforms start looking?
> 
Done.

================
Comment at: tools/llgo/build/context.go:65
@@ +64,3 @@
+		{"openbsd.*", "openbsd"},
+	}
+	match := func(list []REs, s string) string {
----------------
ruiu wrote:
> The regular expressions were written as if they are surrounded by ^ and $, but they are actually not. As a result they look odd -- the trailing .* has no effect, and /k?/ in /k?freebsd/ doesn't have any effect.
Ack. We might just want to remove most of these entries for now and let people add them back in as the compiler is ported to other platforms.

================
Comment at: tools/llgo/cmd/cc-wrapper/main.go:51
@@ +50,3 @@
+	}
+}
+
----------------
ruiu wrote:
> os.StartProcess is a low-level interface, and for this function it doesn't seems to be needed. We should just use os.Command instead. We may be able to replace this function with a few lines of os.Command calls.
Agreed.

================
Comment at: tools/llgo/cmd/gllgo/gllgo.go:49
@@ +48,3 @@
+	fmt.Printf("llgo version %s (%s)\n", llvmVersion(), irgen.GoVersion())
+	fmt.Println()
+	os.Exit(0)
----------------
ruiu wrote:
> nit: just add one more \n to the last Printf instead of calling Println?
Done.

================
Comment at: tools/llgo/cmd/gllgo/gllgo.go:241
@@ +240,3 @@
+				return opts, errors.New("missing path after '-I'")
+			}
+			opts.importPaths = append(opts.importPaths, args[1])
----------------
ruiu wrote:
> So you check for errors for -I but not for -D and other options?
Good catch, added checks for the other options.

================
Comment at: tools/llgo/cmd/gllgo/gllgo.go:493
@@ +492,3 @@
+func getDataInlineAsm(data []byte) string {
+	edata := make([]byte, len(data)*4+10)
+
----------------
ruiu wrote:
> This function could be simplified if you start with the empty byte array with extra buffer (make([]byte, 0, len(data)*4+10) and append strings using append.
Done.

================
Comment at: tools/llgo/debug/debug.go:387
@@ +386,3 @@
+func (d *DIBuilder) descriptorMap(t *types.Map, name string) llvm.Value {
+	return d.descriptorBasic(types.Typ[types.Uintptr], name)
+}
----------------
ruiu wrote:
> Is this TODO?
Perhaps. I am not sure what the debug info for these should look like. @axw was the original author of this code.

================
Comment at: tools/llgo/debug/debug.go:391
@@ +390,3 @@
+func (d *DIBuilder) descriptorChan(t *types.Chan, name string) llvm.Value {
+	return d.descriptorBasic(types.Typ[types.Uintptr], name)
+}
----------------
ruiu wrote:
> ditto
Ditto

http://reviews.llvm.org/D6327






More information about the llvm-commits mailing list