Skip to content

Update PHI node example in README to use more idiomatic structure #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 6, 2017

Conversation

Jumhyn
Copy link
Contributor

@Jumhyn Jumhyn commented May 5, 2017

When using LLVM's PHI nodes, creating a local variable to store the result in each preceding branch defeats the purpose of using PHI nodes in the first place (or, at the very least, obscures their purpose). With the use of local in the example originally, the program could be trivially rewritten as:

define double @calculateFibs(i1) {
entry:
  %local = alloca double
  %1 = icmp ne i1 %0, false
  br i1 %1, label %then, label %else

then:                                             ; preds = %entry
  store double 0x3F8702E05C0B8170, double* %local
  br label %merge

else:                                             ; preds = %entry
  store double 0x3F82C9FB4D812CA0, double* %local
  br label %merge

merge:                                            ; preds = %else, %then
  %retVal = load double, double* %local
  ret double %retVal
}

which sidesteps the need for a PHI node at all. I've rewritten the example to remove the unused local variable and to make the power of PHI nodes clearer.

@Jumhyn Jumhyn changed the title Update PHI node example in README to use proper semantics Update PHI node example in README to use more idiomatic structure May 5, 2017
@CodaFi
Copy link
Member

CodaFi commented May 6, 2017

Ah, true.

Could you update the comments that reference local too?

@CodaFi
Copy link
Member

CodaFi commented May 6, 2017

Better yet, do what I should have done here and store the PHI'd result into the local and return that. The goal isn't necessarily efficiency, but parity with the Swift example above this one.

@Jumhyn
Copy link
Contributor Author

Jumhyn commented May 6, 2017

Updated. Kept the comments since now thenVal and elseVal really do represent the values being stored to local.

@CodaFi
Copy link
Member

CodaFi commented May 6, 2017

Thanks!

@CodaFi CodaFi merged commit ca83e1c into llvm-swift:master May 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants