Skip to content

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