[Kaleidoscope] Fix ForExprAST::codegen

[vmishelcs]

This PR is to address issue #13638.

Background

The Kaleidoscope tutorial has a bug in its approach to generate LLVM IR for for-loops introduced in Chapter 5.

Given input

extern putchard(ch);
def printstar(n) for i = 1, i < n, 1.0 in putchard(42);

the tutorial explains that we should be generating the following IR.

declare double @putchard(double)

define double @printstar(double %n) {
entry:
  ; initial value = 1.0 (inlined into phi)
  br label %loop

loop:       ; preds = %loop, %entry
  %i = phi double [ 1.000000e+00, %entry ], [ %nextvar, %loop ]
  ; body
  %calltmp = call double @putchard(double 4.200000e+01)
  ; increment
  %nextvar = fadd double %i, 1.000000e+00

  ; termination test
  %cmptmp = fcmp ult double %i, %n
  %booltmp = uitofp i1 %cmptmp to double
  %loopcond = fcmp one double %booltmp, 0.000000e+00
  br i1 %loopcond, label %loop, label %afterloop

afterloop:      ; preds = %loop
  ; loop always returns 0.0
  ret double 0.000000e+00
}

However, the IR above has a logic error. In particular, we enter the loop even if we are not supposed to. For instance, if we call printstar(0);, we still print a star because the loop condition is not checked until the end of the loop body.

ready> extern putchard(ch);
ready> Read extern: declare double @putchard(double)

ready> def printstar(n) for i = 1, i < n, 1.0 in putchard(42);
ready> Read function definition:define double @printstar(double %n) {
entry:
  br label %loop

loop:                                             ; preds = %loop, %entry
  %i = phi double [ 1.000000e+00, %entry ], [ %nextvar, %loop ]
  %calltmp = call double @putchard(double 4.200000e+01)
  %nextvar = fadd double %i, 1.000000e+00
  %cmptmp = fcmp ult double %i, %n
  br i1 %cmptmp, label %loop, label %afterloop

afterloop:                                        ; preds = %loop
  ret double 0.000000e+00
}

ready> printstar(1);
ready> *Evaluated to 0.000000

Solution

This PR proposes a fix for this error, and the following IR dump demonstrates the changes.

declare double @putchard(double)

define double @printstar(double %n) {
entry:
  ; initial value = 1.0 (inlined into phi)
  br label %loopcond

loopcond:   ; preds = %loop, %entry
  %i = phi double [ 1.000000e+00, %entry ], [ %nextvar, %loop ]
  ; termination test
  %cmptmp = fcmp ult double %i, %n
  %booltmp = uitofp i1 %cmptmp to double
  %endcond = fcmp one double %booltmp, 0.000000e+00
  br i1 %endcond, label %loop, label %endloop

loop:       ; preds = %loopcond
  ; body
  %calltmp = call double @putchard(double 4.200000e+01)
  ; increment
  %nextvar = fadd double %i, 1.000000e+00
  br label %loopcond

endloop:    ; preds = %loopcond
  ; loop always returns 0.0
  ret double 0.000000e+00
}

With this LLVM IR, here's some example output.

eady> extern putchard(ch);
ready> Read extern: declare double @putchard(double)

ready> def printstar(n) for i = 1, i < n, 1.0 in putchard(42);
ready> Read function definition:define double @printstar(double %n) {
entry:
  ; initial value = 1.0 (inlined into phi)
  br label %loopcond

loopcond:                                         ; preds = %loop, %entry
  %i = phi double [ 1.000000e+00, %entry ], [ %nextvar, %loop ]
  ; termination test
  %cmptmp = fcmp ult double %i, %n
  %booltmp = uitofp i1 %cmptmp to double
  %endcond = fcmp one double %booltmp, 0.000000e+00
  br i1 %endcond, label %loop, label %endloop

loop:                                             ; preds = %loopcond
  ; body
  %calltmp = call double @putchard(double 4.200000e+01)
  ; increment
  %nextvar = fadd double %i, 1.000000e+00
  br label %loopcond

endloop:                                          ; preds = %loopcond
  ; loop always returns 0.0
  ret double 0.000000e+00
}

ready> printstar(1);
ready> Evaluated to 0.000000

In addition to how the for-loop IR is generated, I've updated the tutorial RST files for Chapter 5 and 7 with commentary on my implementation of the ForExprAST::codegen method.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[vmishelcs]

@Logikable @aeubanks @MaskRay @EugeneZelenko @Endilll

Please take a look whenever you find time. Thanks!

[Logikable]

Great catch :) I made the same fix when doing the tutorial, so I was able to compare it with yours for correctness (yours is cleaner!)

I left a couple of ideas for things you could change. Thanks for contributing to the tutorial.

[vmishelcs]

I removed the final sentence of this paragraph because I realized that the tutorial never actually does anything with the for-loop return value in future chapters.

[vmishelcs]

@Logikable Please take a look at the changes I've made, as per your instructions. The build is failing, but I see that the Kaleidoscope tests are passing, so I assume it was because I merged main into my branch before pushing and there is an issue with some of the code in main. Could you take a look at the build logs to confirm I am interpreting them correctly?

Also as a quick question, do you know by any chance why when running the Kaleidoscope tests on my machine, it says they are UNSUPPORTED?

$ build/bin/llvm-lit llvm/test/Examples/Kaleidoscope/

Testing: 5 tests, 5 workers --
UNSUPPORTED: LLVM :: Examples/Kaleidoscope/Chapter4.test (1 of 5)
UNSUPPORTED: LLVM :: Examples/Kaleidoscope/Chapter5.test (2 of 5)
UNSUPPORTED: LLVM :: Examples/Kaleidoscope/Chapter3.test (3 of 5)
UNSUPPORTED: LLVM :: Examples/Kaleidoscope/Chapter7.test (4 of 5)
UNSUPPORTED: LLVM :: Examples/Kaleidoscope/Chapter6.test (5 of 5)

Testing Time: 0.01s

Total Discovered Tests: 5
Unsupported: 5 (100.00%)

Let me know if there are any other issues with my PR! 🙂

[Logikable]

LGTM :)

Let's get someone with write permission to review this. @aeubanks @alinas