[GSoC] LLVM IR dumping #6135

coodie · 2017-04-28T18:12:50Z

This PR adds convenience functions for dumping LLVM IR of given chapel function.

Need for such functions occurred when I started to work on adding LLVM vectorization tests for chapel test suite. They might also be useful in further work on LLVM.

Even very tiny chapel source files generate very big LLVM IR, when working with LLVM we usually need to look only at a piece of code. It seems reasonable to abstract our code into a function and then dump IR of function. There is another problem of chapel internal types, because they are in global space and thus cannot be enclosed in function, but I see it as another problem.

Compiler runs many transformations on LLVM IR, and sometimes we'd like to know if some optimizations occured or not (for example, was this loop vectorized?). At this moment there are 3 possibly useful places where we'd like to dump part of our code:

Non-optimized - before any kind of optimization of function occurs. Just raw, compiler generated IR.
Basic optimization - there are some basic optimizations run on a function after it is generated.
Big optimization - After all big, whole modular, complicated optimization passes are run.

There might be a possibility and a need to add more granularity to 3rd point, like after which pass should we exactly dump a function.

mppf · 2017-04-28T18:42:28Z

I think it would be better, in this use case, if you could print to stdout. Also the LLVM docs say ->dump is for debugging. Is there a 'print' method of some sort you could use instead?

mppf · 2017-04-28T18:44:07Z

compiler/codegen/symbol.cpp

+char llvmFuncDumpName[FUNC_NAME_MAX+1] = "";
+int llvmFuncOptDump = 0;
+
+void llvmFunctionDump(int optLevel, const std::string &name) {


I think this function has the wrong arguments. I think it should accept a Chapel Function* and an llvm::Function*.
Then, compare llvmFuncDumpName against chapelFunction->name and chapelFunction->cname.

mppf · 2017-05-01T17:13:02Z

compiler/codegen/symbol.cpp

@@ -43,7 +43,6 @@

 // LLVM debugging support
 #include "llvmDebug.h"
-


Let's put this space back, since it doesn't have to do with your intended change set.

mppf · 2017-05-01T17:13:15Z

compiler/codegen/symbol.cpp

@@ -75,6 +102,7 @@ void Symbol::codegenDef() {

 void Symbol::codegenPrototype() { }

+


Let's take this space away.

mppf · 2017-05-01T17:13:48Z

compiler/codegen/symbol.cpp

@@ -1261,6 +1289,9 @@ void FnSymbol::codegenDef() {
  body->codegen();
  flushStatements();

+#ifdef HAVE_LLVM
+#endif
+


Take out this ifdef doing nothing?

mppf · 2017-05-01T17:19:08Z

compiler/codegen/symbol.cpp

+
+#ifdef HAVE_LLVM
+void llvmFunctionDump(int optLevel, llvm::Function *llvmFunc, FnSymbol *chapelFunc) {
+  static llvm::Function *func = NULL; //Store function once we've found it


This is really so that you can make the call in makeBinaryLLVM work nicely, right? Maybe put a comment to that effect?

mppf · 2017-05-01T17:23:02Z

compiler/include/symbol.h

+extern int llvmFuncOptDump;
+
+#ifdef HAVE_LLVM
+void llvmFunctionDump(int optLevel, llvm::Function *llvmFunc = NULL, FnSymbol *chapelFunc = NULL);


Let's put a comment here saying what this function does. Something like

Prints out LLVM IR for the function selected by llvmFuncDumpName at the optimization level llvmFuncOptDump. If llvmFunc and chapelFunc are NULL, this function assumes it has been called previously with non-NULL arguments for these functions.

But I find this behavior a little bit odd and wonder if it would be better to just make llvmFunctionDump's static variable in to a global. Is that what you had before? Sorry if I'm leading you in a circle..

mppf · 2017-05-01T17:25:58Z

compiler/main/driver.cpp

@@ -704,6 +704,8 @@ static ArgumentDescription arg_desc[] = {
 {"", ' ', NULL, "LLVM Code Generation Options", NULL, NULL, NULL, NULL},
 {"llvm", ' ', NULL, "[Don't] use the LLVM code generator", "N", &llvmCodegen, "CHPL_LLVM_CODEGEN", NULL},
 {"llvm-wide-opt", ' ', NULL, "Enable [disable] LLVM wide pointer optimizations", "N", &fLLVMWideOpt, "CHPL_LLVM_WIDE_OPTS", NULL},
+ {"llvm-fdump", ' ', "<name>", "Dump LLVM Intermediate Representation of given function to stdout", "S256", llvmFuncDumpName, "CHPL_LLVM_FDUMP", NULL},
+ {"llvm-fdump-opt", ' ', "<opt>", "Specifies from which LLVM transformation phase to print function: 0 - before any transformation, 1 - basic cleaning, 2 - full optimization", "I", &llvmFuncOptDump, "CHPL_LLVM_OPT_FDUMP", NULL},


I think it might be more reasonable for llvm-fdump-opt to take in a string, so that if we add/change optimization levels we don't have to renumber & change tests. Also I might prefer different flag names.

I might like
--print-llvm-ir instead of llvm-fdump and I'm not sure what to call llvm-fdump-opt. Maybe print-llvm-ir-after and then it can take start simplify and optimize, for example?

(I think we'll want to ask some others about that part so make sure to put the user interface for this feature in your PR message. Generally the user interface will receive more scrutiny than the change itself).

Since this needs attention of other developers, I'm not going to focus on that part. I'll just write tests and change them later accordingly.

Right, but could you summarize the usage of the new feature in an email to chapel-developers and/or in the PR message?

Sure. I'll write all down in PR message and then send e-mail pointing to this PR. I'm not sure which is preferred one (e-mail or PR), but I suspect most of devs don't look that often into PR section. Whenever I'm doing these tasks I make quiet assumption that you inform the rest of developers.

mppf · 2017-05-01T17:26:11Z

compiler/util/clangUtil.cpp

@@ -1867,6 +1867,7 @@ void makeBinaryLLVM(void) {
  output.keep();
  output.os().flush();

+


Let's remove this new space.

…unction

coodie · 2017-05-02T02:06:07Z

I've run into small Issues with figuring out simple IR testcase where non-optimized version would differ from simplify optimization. As far as I understood code there is some dataLayout pass which is architecture dependent, and -O2 pass on a function (yes! the one from clang), more details in prepareCodegenLLVM in clangUtil.cpp. For some reason these 'basic' optimizations don't occur. This loop however:

proc test()
{
  forall i in 1..100
  {
    writeln(i);
  }
}

Shows some differences, but they are very minor and generated IR takes ~400 lines of unreadable and difficult to understand code, so I decided not to use it for testing this feature.

mppf · 2017-05-02T13:45:15Z

compiler/codegen/symbol.cpp

@@ -1204,6 +1222,9 @@ void FnSymbol::codegenDef() {
 #ifdef HAVE_LLVM
    func = getFunctionLLVM(cname);

+    if(strcmp(llvmFuncDumpName, name) == 0)
+        llvmFuncDumpCName = strdup(cname);


Because of the way the Chapel compiler manages strings, you don't need to call strdup here. Just make it point to cname.

mppf · 2017-05-02T13:46:17Z

compiler/codegen/symbol.cpp

@@ -1281,11 +1302,17 @@ void FnSymbol::codegenDef() {
        INT_FATAL("LLVM function verification failed");
      }
    }
+
+    if(llvmFuncOptDump == 0 && strcmp(llvmFuncDumpName, name) == 0)


I think it would be nice if 0 meant no dumping, to save some strcmparing in the common case

mppf · 2017-05-02T13:51:44Z

Thanks for the improvements! What we need to do next:

get feedback on the new compile flags. I'll send an email about that.
simplify your tests even more

About simplifying the tests even more, in this case I'm not too worried if the output differs for different optimization levels. Let's test it with as trivial functions as are possible.

hzhang86 · 2017-05-02T14:04:37Z

I prefer the flags:

--print-llvm-ir --print-llvm-ir-after

where is:
* none -- before any optimization
* basic -- after basic simplification
* full -- after full optimization

That's just my own favour.

bradcray · 2017-05-03T16:22:40Z

Personally, I'd probably choose the names --llvm-print-* rather than --print-llvm-* just to put the llvm-specific aspect front-and-center, but I don't feel strongly about this if you disagree. I also agree that the mnemonic flag settings rather than integers seem more attractive.

mppf · 2017-05-03T16:48:57Z

OK, so if we combine all of the thoughts, we end up with:

 --llvm-print-ir <function-name> --llvm-print-ir-after <opt-name>

where <opt-name> is:
    * none -- before any optimization
    * basic -- after basic simplification
    * full -- after full optimization

coodie · 2017-05-05T16:05:08Z

Well I the only thing that comes to my mind would be change --llvm-print-ir-after to --llvm-print-ir-stage, but that's not a big deal.

And when it comes to testing I've came up with simple idea, which would make tests a bit more reasonable. When we dump LLVM IR representation I guess it would be good to add comment in printed LLVM IR saying from which optimization stage the printed IR comes from. Like:

$ chpl --llvm --llvm-print-ir mainTest --llvm-print-ir-after basic test.chpl

; Dump of function mainTest after basic optimizations
define internal void @mainTest_chpl(i64 %n_chpl) {
entry:
  %chpl_macro_tmp_713 = alloca i64
  store i64 %n_chpl, i64* %chpl_macro_tmp_713, !tbaa !0
  %_ic__F1_high_chpl = alloca i64
  %i_chpl = alloca i64
  br label %mainTest_chpl_2blk_body_
...

Then in tests we can check if we printed that line. It doesn't of course ensure that printed IR is actually from stage, but provides extra info to the user and can be good starting point for correctness tests.

mppf · 2017-05-05T16:07:40Z

I like the strategy to improve the tests. Re the naming, it would be fine with me to have --llvm-print-ir-stage. I think you have enough information to adjust this PR.

…algorithm

mppf · 2017-05-05T19:43:18Z

compiler/main/driver.cpp

@@ -704,8 +709,8 @@ static ArgumentDescription arg_desc[] = {
 {"", ' ', NULL, "LLVM Code Generation Options", NULL, NULL, NULL, NULL},
 {"llvm", ' ', NULL, "[Don't] use the LLVM code generator", "N", &llvmCodegen, "CHPL_LLVM_CODEGEN", NULL},
 {"llvm-wide-opt", ' ', NULL, "Enable [disable] LLVM wide pointer optimizations", "N", &fLLVMWideOpt, "CHPL_LLVM_WIDE_OPTS", NULL},
- {"llvm-fdump", ' ', "<name>", "Dump LLVM Intermediate Representation of given function to stdout", "S256", llvmFuncDumpName, "CHPL_LLVM_FDUMP", NULL},
- {"llvm-fdump-opt", ' ', "<opt>", "Specifies from which LLVM transformation phase to print function: 0 - before any transformation, 1 - basic cleaning, 2 - full optimization", "I", &llvmFuncOptDump, "CHPL_LLVM_OPT_FDUMP", NULL},
+ {"llvm-print-ir", ' ', "<name>", "Dump LLVM Intermediate Representation of given function to stdout", "S256", llvmPrintIrName, "CHPL_LLVM_FDUMP", NULL},


update enviro var names too e.g. "CHPL_LLVM_FDUMP"

mppf · 2017-05-05T19:45:09Z

compiler/codegen/symbol.cpp

@@ -1222,8 +1238,8 @@ void FnSymbol::codegenDef() {
 #ifdef HAVE_LLVM
    func = getFunctionLLVM(cname);

-    if(strcmp(llvmFuncDumpName, name) == 0)
-        llvmFuncDumpCName = strdup(cname);
+    if(strcmp(llvmPrintIrName, name) == 0)


can this be if (llvmPrintIrNameStageNum !=0 && strcmp(...) == 0 ) ?

mppf · 2017-05-05T19:47:23Z

compiler/codegen/symbol.cpp

+    {LLVM_FULL_STAGE_NAME, LLVM_FULL_STAGE_NUM}
+};
+
+std::map<int, std::string> llvmStageRevMap =


std::map seems overkill for these 3-element debugging lists. I'd personally probably just use an array for int -> string and then linear search for the other way... but there's not anything wrong exactly with using std::map.

I have to agree with you that map is a bit of overkill.

In this case it was just easier for me to assign (value)->(key) using std::map notation. Normally I'd like to have std::array or std::vector equivalent looking like this:

std::vector llvmStageRevMap = { [LLVM_NONE_STAGE_NUM] = LLVM_NONE_STAGE_NAME, [LLVM_BASIC_STAGE_NUM] = LLVM_BASIC_STAGE_NAME, [LLVM_FULL_STAGE_NUM] = LLVM_FULL_STAGE_NAME };

But C++ doesn't support such assignment to vector.

I consider to even remove map, and simply use bunch of 'if, else' in places where I actually use this: llvmFunctionDump, and verifyStageAndsetStageNum, but this is just my programming style to use map for this kind of task.

It can just be

char* llvmStageRevMap[] = { "", LLVM_NONE_STAGE_NAME, LLVM_BASIC_STAGE_NAME, LLVM_FULL_STAGE_NAME }

Anybody adding stages will need to modify it anyway

mppf · 2017-05-05T19:48:32Z

test/compflags/coodie/llvmPrintIrBasic.chpl

@@ -0,0 +1,5 @@
+proc mainTest()
+{
+  writeln("Hello World!");


Can it just be an empty proc?

mppf · 2017-05-05T19:49:07Z

test/compflags/coodie/llvmPrintIrBasic.good

+mainTest_chpl_2blk_body_:                         ; preds = %entry
+  %0 = load %string, %string* @_str_literal_1924, !tbaa !0
+  store %string %0, %string* %local__str_literal_1924_chpl, !tbaa !0
+  call void @writeln_chpl2(%string* %local__str_literal_1924_chpl, i64 3, i32 51)


writeln_chpl2 won't be stable as the compiler changes.
Can you put a PREDIFF that just greps out everything except for the "LLVM IR representation" comment?

… can be in any order

mppf · 2017-05-08T14:01:28Z

compiler/codegen/symbol.cpp

@@ -1204,6 +1245,9 @@ void FnSymbol::codegenDef() {
 #ifdef HAVE_LLVM
    func = getFunctionLLVM(cname);

+    if(strcmp(llvmPrintIrName, name) == 0)


Just to be totally clear & to minimize the function call, can this be

if (llvmPrintIrStageNum != llvmStageNum::NOPRINT && strcmp(llvmPrintIrName, name) == 0)

?

mppf · 2017-05-08T14:06:19Z

compiler/include/symbol.h

+extern const char *llvmStageName[llvmStageNum::LAST];
+
+const char *stageNameFromStageNum(int stageNum);
+int stageNumFromStageName(const char* stageName);


Shouldn't these functions be prefixed with 'llvm' e.g. 'llvmStageNameFromStageNum' ?

mppf · 2017-05-08T14:07:59Z

compiler/main/driver.cpp

+static void verifyStageAndSetStageNum(const ArgumentDescription* desc, const char* arg_unused)
+{
+  int stageNum = stageNumFromStageName(llvmPrintIrStage);
+  if(!stageNum)


Everywhere else, stageNum is only ever one of the enum values. So shouldn't this say

if (stageNum == llvmStageNum::NOPRINT)

?

mppf · 2017-05-08T14:08:22Z

compiler/include/symbol.h

+extern int llvmPrintIrStageNum;
+
+namespace llvmStageNum {
+enum { NOPRINT = 0,


I'd consider making this a typedef enum and then using that type instead of int for llvmPrintIrStageNum.

mppf · 2017-05-08T14:09:04Z

man/chpl.rst

+**--llvm-print-ir-stage <stage>**
+    Picks stage from which to print LLVM IR of function defined in 
+    **--llvm-print-ir**. 
+    Chapel compiler runs many different optimization passes each of which


Add a The -> "The chapel compiler runs..."

mppf · 2017-05-08T14:09:57Z

man/chpl.rst

+    Picks stage from which to print LLVM IR of function defined in 
+    **--llvm-print-ir**. 
+    Chapel compiler runs many different optimization passes each of which
+    can change IR of function. This option allows to pick IR of function


"can change IR of function" -> "can change the IR of functions"

"allows to pick IR" -> "allows one to pick the IR"

mppf · 2017-05-08T18:58:04Z

I ran full local testing and got these failures:

[Error matching compiler output for compflags/bradc/help/userhelp]
[Error matching compiler output for compflags/bradc/printstuff/zall]
[Error matching compiler output for compflags/bradc/printstuff/zcopyhelp]
[Error matching compiler output for compflags/bradc/printstuff/zhelplice]
[Error matching compiler output for compflags/bradc/printstuff/zhelpvers]

mppf · 2017-05-09T14:36:13Z

Passed full local testing. I needed to make some space changes to chpl.rst in order to get make docs to work. I plan to merge and then commit the space changes.

Follow-on to PR chapel-lang#6135.

Adjust spacing in chpl.rst to be valid rst Follow-on to PR #6135 so that `make docs` completes without error.

bradcray · 2017-09-21T18:33:16Z

compiler/main/driver.cpp

@@ -704,6 +713,8 @@ static ArgumentDescription arg_desc[] = {
 {"", ' ', NULL, "LLVM Code Generation Options", NULL, NULL, NULL, NULL},
 {"llvm", ' ', NULL, "[Don't] use the LLVM code generator", "N", &llvmCodegen, "CHPL_LLVM_CODEGEN", NULL},
 {"llvm-wide-opt", ' ', NULL, "Enable [disable] LLVM wide pointer optimizations", "N", &fLLVMWideOpt, "CHPL_LLVM_WIDE_OPTS", NULL},
+ {"llvm-print-ir", ' ', "<name>", "Dump LLVM Intermediate Representation of given function to stdout", "S256", llvmPrintIrName, "CHPL_LLVM_PRINT_IR", NULL},
+ {"llvm-print-ir-stage", ' ', "<stage>", "Specifies from which LLVM optimization stage to print function: none, basic, full", "S256", llvmPrintIrStage, "CHPL_LLVM_PRINT_IR_STAGE", &verifyStageAndSetStageNum},


@coodie and @mppf: In putting together the release CHANGES file, I only noticed these flags for the first time. These strike me as developer, rather than user, flags. Is there an argument for keeping them in user space rather than pushing them down into developer space? (the arguments for pushing them down being to keep the --help output and man page slightly shorter). Thanks.

The argument for having them in user space is "Users might want to see the LLVM IR output" but I think it'd be reasonable to call such users "developers", whatever that means. I'll make a PR to move it down.

@bradcray

Make --llvm-print-ir a developer flag Suggested by @bradcray. PR #6135 added --llvm-print-ir and documented it as a user feature... But this feature is really only useful to developers or expert users who might used developer flags anyway. Passed full local testing.

coodie changed the title ~~Llvm fdump~~ LLVM IR dumping Apr 28, 2017

coodie added 3 commits April 28, 2017 20:39

Add --llvm-fdump and --llvm-opt-fdump options

b4452f5

Add generating on different optimization levels

0d5bf03

Add missing HAVE_LLVM

dfb9a04

mppf reviewed Apr 28, 2017

View reviewed changes

coodie added 3 commits May 1, 2017 15:04

llvmFunctionDump prints to stdOut

67dce1f

Print to stdout and refactor

6375016

Add missing HAVE_LLVM

ace64f7

mppf reviewed May 1, 2017

View reviewed changes

coodie added 3 commits May 1, 2017 20:31

Algorithm for dumping is now based on global variable of C name for f…

e4d97b2

…unction

Remove unnecessary spaces

b6e9f86

Add tests

af450fb

mppf reviewed May 2, 2017

View reviewed changes

coodie added 2 commits May 5, 2017 20:26

Change --llvm-fdump to --llvm-print-ir, remove tests, modify dumping …

5708577

…algorithm

Add tests, update printed message, error when wrong name given

957a12f

mppf reviewed May 5, 2017

View reviewed changes

coodie added 7 commits May 5, 2017 21:58

This is not camelcase!

28eccc2

Add .prediff for tests

4be6a2f

Add PREDIFF, refactor to use enums, remove maps

8d0c369

Move stageNames initialization to function

3ab4d47

Stage names have to be initizalied statically, because compiler flags…

f38b56c

… can be in any order

Remove unnecessary function

5b59d2e

Update manpage

7a6717b

mppf reviewed May 8, 2017

View reviewed changes

coodie added 2 commits May 8, 2017 22:06

Introduce llvmStageNum_t, add minor code and man improvements

471656a

Update userhelp.good

883a119

mppf merged commit 8d704f0 into chapel-lang:master May 9, 2017

mppf added a commit to mppf/chapel that referenced this pull request May 9, 2017

Adjust spacing in chpl.rst to be valid rst

d82ddd9

Follow-on to PR chapel-lang#6135.

mppf mentioned this pull request May 9, 2017

Adjust spacing in chpl.rst to be valid rst #6199

Merged

mppf added a commit that referenced this pull request May 9, 2017

Merge pull request #6199 from mppf/fix-make-docs

f6b9e4f

Adjust spacing in chpl.rst to be valid rst Follow-on to PR #6135 so that `make docs` completes without error.

coodie changed the title ~~LLVM IR dumping~~ [GSoC] LLVM IR dumping Aug 28, 2017

bradcray reviewed Sep 21, 2017

View reviewed changes

mppf mentioned this pull request Sep 21, 2017

Make --llvm-print-ir a developer flag #7427

Merged

		@@ -43,7 +43,6 @@

		// LLVM debugging support
		#include "llvmDebug.h"

		@@ -75,6 +102,7 @@ void Symbol::codegenDef() {

		void Symbol::codegenPrototype() { }

		@@ -1867,6 +1867,7 @@ void makeBinaryLLVM(void) {
		output.keep();
		output.os().flush();

[GSoC] LLVM IR dumping #6135

[GSoC] LLVM IR dumping #6135

Conversation

coodie commented Apr 28, 2017 • edited Loading

mppf commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coodie commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mppf commented May 2, 2017

hzhang86 commented May 2, 2017

bradcray commented May 3, 2017

mppf commented May 3, 2017 • edited Loading

coodie commented May 5, 2017

mppf commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coodie May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mppf May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mppf commented May 8, 2017

mppf commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coodie commented Apr 28, 2017 •

edited

Loading

mppf commented May 3, 2017 •

edited

Loading

coodie May 5, 2017 •

edited

Loading

mppf May 8, 2017 •

edited

Loading