Page MenuHomePhabricator

Initial contribution of llgo, a Go frontend
ClosedPublic

Authored by pcc on Nov 19 2014, 1:51 PM.

Details

Reviewers
chandlerc
ruiu
pcc

Diff Detail

Event Timeline

pcc updated this revision to Diff 16398.Nov 19 2014, 1:51 PM
pcc retitled this revision from to Initial contribution of llgo, a Go frontend.
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added reviewers: ruiu, chandlerc.
pcc added a subscriber: Unknown Object (MLST).
ruiu edited edge metadata.Nov 19 2014, 2:10 PM

I'll read this patch as much as possible and try to get the whole picture. But because much of the code is already reviewed by llgo developers, I don't assume that the same level of code review as other patches is needed. I'll mainly look for obvious red signals (if exists).

chandlerc edited edge metadata.Nov 19 2014, 2:16 PM

So, I think the important question here is -- should this go (ba-dum!) in a
separate subproject much the way clang, lld, and lldb do? I think the
answer is "yes".

CC-ing Chris for comment on this.

2014-11-19 15:51 GMT-06:00 Peter Collingbourne <peter@pcc.me.uk>:

Hi ruiu, chandlerc,

http://reviews.llvm.org/D6327

Files:

cmake/modules/AddLLVM.cmake
tools/CMakeLists.txt
tools/llgo/.gitignore
tools/llgo/CMakeLists.txt
tools/llgo/LICENSE.TXT
tools/llgo/README.TXT
tools/llgo/build/context.go
tools/llgo/cmd/cc-wrapper/main.go
tools/llgo/cmd/gllgo/gllgo.go
tools/llgo/debug/debug.go
tools/llgo/irgen/annotations.go
tools/llgo/irgen/attribute.go
tools/llgo/irgen/builtins.go
tools/llgo/irgen/cabi.go
tools/llgo/irgen/call.go
tools/llgo/irgen/channels.go
tools/llgo/irgen/closures.go
tools/llgo/irgen/compiler.go
tools/llgo/irgen/errors.go
tools/llgo/irgen/indirect.go
tools/llgo/irgen/interfaces.go
tools/llgo/irgen/maps.go
tools/llgo/irgen/parser.go
tools/llgo/irgen/predicates.go
tools/llgo/irgen/println.go
tools/llgo/irgen/runtime.go
tools/llgo/irgen/slice.go
tools/llgo/irgen/ssa.go
tools/llgo/irgen/strings.go
tools/llgo/irgen/targets.go
tools/llgo/irgen/typemap.go
tools/llgo/irgen/types.go
tools/llgo/irgen/utils.go
tools/llgo/irgen/value.go
tools/llgo/irgen/version.go
tools/llgo/libgo-noext.diff
tools/llgo/llgo-go.sh
tools/llgo/ssaopt/esc.go
tools/llgo/test/CMakeLists.txt
tools/llgo/test/debuginfo/emptyname.go
tools/llgo/test/execution/Inputs/init2.go
tools/llgo/test/execution/arrays/compare.go
tools/llgo/test/execution/arrays/index.go
tools/llgo/test/execution/arrays/range.go
tools/llgo/test/execution/arrays/slice.go
tools/llgo/test/execution/assignment/arrays.go
tools/llgo/test/execution/assignment/binop.go
tools/llgo/test/execution/assignment/dereferencing.go
tools/llgo/test/execution/assignment/multi.go
tools/llgo/test/execution/assignment/namedresult.go
tools/llgo/test/execution/branching/goto.go
tools/llgo/test/execution/branching/labeled.go
tools/llgo/test/execution/chan/buffered.go
tools/llgo/test/execution/chan/range.go
tools/llgo/test/execution/chan/select.go
tools/llgo/test/execution/chan/self.go
tools/llgo/test/execution/circulartype.go
tools/llgo/test/execution/closures/basic.go
tools/llgo/test/execution/closures/issue176.go
tools/llgo/test/execution/complex.go
tools/llgo/test/execution/const.go
tools/llgo/test/execution/conversions/complex.go
tools/llgo/test/execution/conversions/float.go
tools/llgo/test/execution/conversions/int.go
tools/llgo/test/execution/conversions/sameunderlying.go
tools/llgo/test/execution/defer.go
tools/llgo/test/execution/errors/recover.go
tools/llgo/test/execution/for/branch.go
tools/llgo/test/execution/fun.go
tools/llgo/test/execution/functions/compare.go
tools/llgo/test/execution/functions/multivalue.go
tools/llgo/test/execution/functions/unreachable.go
tools/llgo/test/execution/go.go
tools/llgo/test/execution/if/lazy.go
tools/llgo/test/execution/init.go
tools/llgo/test/execution/interfaces/assert.go
tools/llgo/test/execution/interfaces/basic.go
tools/llgo/test/execution/interfaces/comparei2i.go
tools/llgo/test/execution/interfaces/comparei2v.go
tools/llgo/test/execution/interfaces/e2i_conversion.go
tools/llgo/test/execution/interfaces/embedded.go
tools/llgo/test/execution/interfaces/error.go
tools/llgo/test/execution/interfaces/i2i_conversion.go
tools/llgo/test/execution/interfaces/import.go
tools/llgo/test/execution/interfaces/methods.go
tools/llgo/test/execution/interfaces/static_conversion.go
tools/llgo/test/execution/interfaces/wordsize.go
tools/llgo/test/execution/literals/array.go
tools/llgo/test/execution/literals/func.go
tools/llgo/test/execution/literals/map.go
tools/llgo/test/execution/literals/slice.go
tools/llgo/test/execution/literals/struct.go
tools/llgo/test/execution/maps/delete.go
tools/llgo/test/execution/maps/insert.go
tools/llgo/test/execution/maps/lookup.go
tools/llgo/test/execution/maps/range.go
tools/llgo/test/execution/methods/methodvalues.go
tools/llgo/test/execution/methods/nilrecv.go
tools/llgo/test/execution/methods/selectors.go
tools/llgo/test/execution/new.go
tools/llgo/test/execution/nil.go
tools/llgo/test/execution/operators/basics.go
tools/llgo/test/execution/operators/binary_untyped.go
tools/llgo/test/execution/operators/shifts.go
tools/llgo/test/execution/slices/append.go
tools/llgo/test/execution/slices/cap.go
tools/llgo/test/execution/slices/compare.go
tools/llgo/test/execution/slices/copy.go
tools/llgo/test/execution/slices/index.go
tools/llgo/test/execution/slices/literal.go
tools/llgo/test/execution/slices/make.go
tools/llgo/test/execution/slices/sliceexpr.go
tools/llgo/test/execution/strings/add.go
tools/llgo/test/execution/strings/bytes.go
tools/llgo/test/execution/strings/compare.go
tools/llgo/test/execution/strings/index.go
tools/llgo/test/execution/strings/range.go
tools/llgo/test/execution/strings/runetostring.go
tools/llgo/test/execution/strings/slice.go
tools/llgo/test/execution/structs/compare.go
tools/llgo/test/execution/structs/embed.go
tools/llgo/test/execution/switch/branch.go
tools/llgo/test/execution/switch/default.go
tools/llgo/test/execution/switch/empty.go
tools/llgo/test/execution/switch/scope.go
tools/llgo/test/execution/switch/strings.go
tools/llgo/test/execution/switch/type.go
tools/llgo/test/execution/types/named.go
tools/llgo/test/execution/types/recursive.go
tools/llgo/test/execution/unsafe/const_sizeof.go
tools/llgo/test/execution/unsafe/offsetof.go
tools/llgo/test/execution/unsafe/pointer.go
tools/llgo/test/execution/unsafe/sizeof_array.go
tools/llgo/test/execution/unsafe/sizeof_basic.go
tools/llgo/test/execution/unsafe/sizeof_struct.go
tools/llgo/test/execution/var.go
tools/llgo/test/execution/varargs.go
tools/llgo/test/gllgo/dead.go
tools/llgo/test/irgen/mangling.go
tools/llgo/test/lit.cfg
tools/llgo/test/lit.site.cfg.in
tools/llgo/update_third_party.sh
tools/llgo/utils/benchcomp/README
tools/llgo/utils/benchcomp/analyze.R
tools/llgo/utils/benchcomp/main.go
tools/llvm-go/llvm-go.go

llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

silvas added a subscriber: silvas.Nov 19 2014, 2:28 PM
silvas added inline comments.
tools/llgo/test/execution/assignment/namedresult.go
2–5

If I'm understanding this use of go run correctly, this seems like a very strange testing strategy for the LLVM project.

It would be as though clang's tests used gcc or msvc as a testing tool and only tested the output of compiled programs.

Is there a reason you haven't organized your testing like clang? I.e. verifying the compiler's IRGen and diagnostics?

pcc added inline comments.Nov 19 2014, 3:24 PM
tools/llgo/test/execution/assignment/namedresult.go
2–5

Most of the tests in test/execution are from an earlier stage of the project when it was more experimental. I agree with you -- at this point, there is no excuse for making them depend on the output of another compiler.

I'll see if I can rewrite them to use FileCheck to check the command output. I also intend to gradually make these tests more focused and move them into the other subdirectories of test, but that will take time.

pcc updated this revision to Diff 16409.Nov 19 2014, 7:15 PM
pcc edited edge metadata.
  • List supported platforms in README
  • Filter libgo_*san targets based on available sanitizers
  • FileCheck-ify the execution tests
tools/llgo/test/execution/assignment/namedresult.go
2–5

Done.

emaste added a subscriber: emaste.Nov 20 2014, 8:06 AM
emaste added inline comments.Nov 20 2014, 8:18 AM
tools/llgo/README.TXT
18–19

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?

axw added a subscriber: axw.Nov 20 2014, 5:49 PM
ruiu added a comment.Nov 20 2014, 6:50 PM

So far it looks pretty good. First round of review.

These comments are mostly nitpicky stuff, and I don't think we want to address them in this patch.If we do, this patch will become a port from the git repostioy plus various different kind of fixes. I left these comments just because I noticed them and didn't want to lose it. If something needs to be addressed, I think it's better to fix in a follow-up patch.

tools/llgo/CMakeLists.txt
45

Why do you use locally built clang to build go? Can't it compile with system's C/C++ compiler?

tools/llgo/build/context.go
66

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.

tools/llgo/cmd/cc-wrapper/main.go
52

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.

tools/llgo/cmd/gllgo/gllgo.go
50

nit: just add one more \n to the last Printf instead of calling Println?

242

So you check for errors for -I but not for -D and other options?

494

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.

tools/llgo/debug/debug.go
388

Is this TODO?

392

ditto

pcc added a comment.Nov 21 2014, 5:03 PM

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

tools/llgo/CMakeLists.txt
45

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.

tools/llgo/README.TXT
18–19

Done.

tools/llgo/build/context.go
66

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.

tools/llgo/cmd/cc-wrapper/main.go
52

Agreed.

tools/llgo/cmd/gllgo/gllgo.go
50

Done.

242

Good catch, added checks for the other options.

494

Done.

tools/llgo/debug/debug.go
388

Perhaps. I am not sure what the debug info for these should look like. @axw was the original author of this code.

392

Ditto

pcc updated this revision to Diff 16522.Nov 21 2014, 5:11 PM
  • Modify the update_third_party script to remove unused GPL licensed files
  • Fix Linuxisms in update_third_party.sh script
  • Add porting and contribution information to the README
  • Address some of Rui's comments
  • Use USES_TERMINAL with check-libgo target
ruiu added inline comments.Nov 24 2014, 2:52 PM
tools/llgo/irgen/annotations.go
23

Is this how gofmt sorted?

tools/llgo/irgen/cabi.go
43

This field doesn't seems to be used at all. Is this needed?

96

Doesn't this have to include the padding after the last member? I mean if we have a struct, say struct { int; char; }, it's size would be calculated as 5 by this function, but I think it should be 8. (If it's 5, sizeof [2]struct{ int; char; } would become 10?)

104

Oh okay, so the function is used only here. Then the thing I pointed out above is not actually a problem.

208

Not very sure what this piece of code does... but is it going to create for example 1 million values if we have [1024 * 1024]sometype in source code?

tools/llgo/irgen/strings.go
73

Does this compare a value of type int as a pointer? stringiter2 returns (int, rune), not (*sometype, rune).

Oh but this may be intentional -- currently, in all architectures sizeof(int) == sizeof(pointer).

pcc updated this revision to Diff 16590.Nov 24 2014, 4:56 PM
  • Give signext/zeroext attributes to integer arguments according to width and signedness
  • Re-gofmt
tools/llgo/irgen/annotations.go
23

Reformatted (here and elsewhere).

tools/llgo/irgen/cabi.go
43

This field ought to be used to control whether certain arguments receive a signext or zeroext attribute, but it seems we've somehow gotten away with not doing this up to now. Fixed.

104

Right.

208

It shouldn't do. This function is only used when passing an argument directly (i.e. in registers), and with the System V x86-64 calling convention that can only happen with arguments of size <= 16 bytes.

tools/llgo/irgen/strings.go
73

See include/llvm/IR/IRBuilder.h line 1469 or thereabouts -- this will compare the given value against the null value of its type, which in this case would just be integer zero.

axw added inline comments.Nov 25 2014, 2:07 AM
tools/llgo/debug/debug.go
388

Yes, sorry, please add a TODO/FIXME. This should be DW_TAG_pointer_type to __go_map.

I ran gllgo over some basic code and the current output isn't great. I'll submit a patch or two once this is in to improve that.

(Sorry, I wrote this comment shortly after seeing the above, didn't realise I hadn't submitted the reply.)

392

ditto :)

pcc updated this revision to Diff 16665.Nov 26 2014, 3:52 PM
  • Add MIT/UIUC dual licensed versions of runtime library headers,
  • Build tweaks
  • Add FIXMEs for debug info
pcc accepted this revision.Nov 26 2014, 3:59 PM
pcc added a reviewer: pcc.

So this should now be ready to check in to the newly created llgo subproject. I've uploaded my final changes here for the record and will close this revision.

This revision is now accepted and ready to land.Nov 26 2014, 3:59 PM
pcc closed this revision.Nov 26 2014, 4:17 PM

Checked in r222857 (llgo), r222858 (third-party), r222860 (LLVM build).

I found there is files that mixed up with tab and spaces, is there any
coding style for go language?
Maybe use 2 space for all is a better idea?