search

llir / llvm

Library for interacting with LLVM IR in pure Go.
https://llir.github.io/document/
BSD Zero Clause License
1.19k stars 78 forks source link

rename Block.Term? #152

Closed dannypsnl closed 4 years ago

dannypsnl commented 4 years ago

I found this is quite useful when generating a control flow structure, users would like to know a block has a terminator(jump after terminator is illegal) or not, then decide needs jump to end block to avoid executing else block.

dannypsnl commented 4 years ago

@mewmew Since we can judge this by Block.Term == nil, but term this name conflict with term/type in type theory, maybe rename it to terminator is enough?

sangisos commented 4 years ago

Basic block is already design to have an obligatory terminator, is it not? Maybe I'm misunderstanding. https://github.com/llir/llvm/blob/47495a98a87689f6cfbf554375fecd0e233fa38b/ir/block.go#L68-L70

dannypsnl commented 4 years ago

https://github.com/llir/llvm/issues/152#issuecomment-706491696

Yes, that's what I'm looking for. As usual, llir doesn't provide the helper method, then the problem would be naming only.

mewmew commented 4 years ago

@mewmew Since we can judge this by Block.Term == nil, but term this name conflict with term/type in type theory, maybe rename it to terminator is enough?

This seems like an unfortunate overlap of naming. I know that term and type are used in the nomenclature of type theory. However, changing from Term to Terminator seems like an invasive change to the llir API as there are several packages which depend on the current naming.

Also note that these kind of issues with naming come up from time to time. For instance, type is a reserved keyword in Go, so any parameter name, variable or method is usually named typ by convention to avoid this collision. It's not the most beautiful, but it is pragmatic and in the spirit of Go.

As such, I would suggest naming Term TTerm or something along those lines, where T could be short for type-theory, e.g. type-theory term. Is't not perfect, but works.

In general, I would like to keep name changes minimal unless we do careful design and more overhauling API changes where we make the API more consistent and perhaps change aspects of how the API is used. So, for the time being, I'll mark this as unfortunate, in a similar fashion as in done in the Go project.

Basic block is already design to have an obligatory terminator, is it not? Maybe I'm misunderstanding.

Indeed, every basic block is required to have a terminator instruction.

Yes, that's what I'm looking for. As usual, llir doesn't provide the helper method, then the problem would be naming only.

Yes, the approach taken by llir is to implement the core functionality of LLVM IR and then let users develop their own helper packages (e.g. irutil). The main reason for this is that unlike core functionality, the evaluation of what makes a good API of helper libraries is very subjective, and as such, it is easy to envision there may exist more than one.

So, for this specific issue, it all boils down to naming, and while unfortunate, it is possible to solve the name collision by using e.g. TTerm in the type-theory package.

Cheers, Robin

dannypsnl commented 4 years ago

Make sense, then a helper like EndWithTerminator in irutil would be fine?

mewmew commented 4 years ago

Make sense, then a helper like EndWithTerminator in irutil would be fine?

I think I need to better understand what this function would do. Would it add a terminator to a basic block if it doesn't have one? If so, would it add a ret terminator? I think I need more context. For the being, I would suggest that you add EndWithTerminator to an irutil package under github.com/dannypsnl/irutil and then we can always merge this functionality into llir/llvm when more users express the same need. Hope that feels ok?

dannypsnl commented 4 years ago

Sure, I already complete what I expected, everyone can check it at https://github.com/dannypsnl/irutil/commit/50efca58d50666def7b405f20d6f0fbc5393d7f7, @mewmew if you think ext package is good to have then I would send a PR. Close.

mewmew commented 4 years ago

Sure, I already complete what I expected, everyone can check it at dannypsnl/irutil@50efca5, @mewmew if you think ext package is good to have then I would send a PR. Close.

Good. I'd say for now, lets keep the IsEndWithTerminator in dannypsnl/irutil/ext. The same functionality is still possible to achieve by checking block.Term != nil for llir.

Cheers, Robin