This is an archive of the discontinued LLVM Phabricator instance.

[PrintSCC] Fix printing a basic-block without a name
ClosedPublic

Authored by ekatz on May 26 2020, 6:11 AM.

Details

Summary

Print a basic-block as an operand to handle the case where it has no name.

Diff Detail

Event Timeline

ekatz created this revision.May 26 2020, 6:11 AM
ekatz updated this revision to Diff 266646.May 27 2020, 1:19 PM

Added a test case.

fhahn accepted this revision.May 28 2020, 10:17 AM

LGTM, thanks!

llvm/test/Other/print-cfg-sccs.ll
21

nit: branch on undef is UB. It would probably be better to pass in a condition as argument or something like that.

This revision is now accepted and ready to land.May 28 2020, 10:17 AM
ekatz marked an inline comment as done.May 28 2020, 12:23 PM
ekatz added inline comments.
llvm/test/Other/print-cfg-sccs.ll
21

You are correct of course, but I have seen many other test cases that take the undef approach where all that matters is the CFG, and not the contents. for example "loop-pass-ordering.ll" (in the same directory).

baziotis added inline comments.May 28 2020, 1:19 PM
llvm/test/Other/print-cfg-sccs.ll
21

I'm not familiar with a big part of LLVM test-suite, but any test cases that branch on undef (or generally, that they have some undef or whatever UB) it was intended for specific reasons (for example, to test UB recognition or it was propagated somehow).
I also think that it would be better to use a valid value. Otherwise it looks good :)

ekatz marked an inline comment as done.May 28 2020, 8:26 PM
ekatz added inline comments.
llvm/test/Other/print-cfg-sccs.ll
21

As far as I know, it is ok to use undef in a brach if the destination doesn't matter.
Anyway, I think there is no need to argue, as there is a very easy fix for it - I'll just add a function argument for the condition.

fhahn added inline comments.May 29 2020, 8:24 AM
llvm/test/Other/print-cfg-sccs.ll
21

As far as I know, it is ok to use undef in a brach if the destination doesn't matter.

IIRC some passes had/have this interpretation, but it was recently clarified in the LangRef to make branch on undef UB explicitly (rG05f0e598ab265a80fedb23225cde4176f11774ac)

Anyway, I think there is no need to argue, as there is a very easy fix for it - I'll just add a function argument for the condition.

Thanks!

This revision was automatically updated to reflect the committed changes.