This is an archive of the discontinued LLVM Phabricator instance.

IR: Support parsing numeric block ids, and emit them in textual output.
ClosedPublic

Authored by jyknight on Feb 22 2019, 7:23 AM.

Details

Summary

Just as as llvm IR supports explicitly specifying numeric value ids
for instructions, and emits them by default in textual output, now do
the same for blocks.

This is a slightly incompatible change in the textual IR format.

Previously, llvm would parse numeric labels as string names. E.g.

define void @f() {
  br label %"55"
55:
  ret void
}

defined a label *named* "55", even without needing to be quoted, while
the reference required quoting. Now, if you intend a block label which
looks like a value number to be a name, you must quote it in the
definition too (e.g. "55":).

Previously, llvm would print nameless blocks only as a comment, and
would omit it if there was no predecessor. This could cause confusion
for readers of the IR, just as unnamed instructions did prior to the
addition of "%5 = " syntax, back in 2008 (PR2480).

Now, it will always print a label for an unnamed block, with the
exception of the entry block. (IMO it may be better to print it for
the entry-block as well. However, that requires updating many more
tests.)

Thus, the following is supported, and is the canonical printing:

define i32 @f(i32, i32) {
  %3 = add i32 %0, %1
  br label %4

4:
  ret i32 %3
}

New test cases covering this behavior are added, and other tests
updated as required.

Diff Detail

Repository
rC Clang

Event Timeline

jyknight created this revision.Feb 22 2019, 7:23 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 22 2019, 7:23 AM

I like this idea, and I don’t think the textual IR central is too important. A few things:

  • Changes to the IR should always be discussed on llvm-dev. Did this already happen?
  • Please update LangRef.
  • Did you write a script for updating the test cases? If so, you might consider attaching it to the RFC and linking to it from the commit message as a courtesy to downstream maintainers.

I like this idea, and I don’t think the textual IR central is too important.

Textual IR *change*; typing on a phone while walking :/. By which I mean that IMO it’s fine to break/change this.

+1. Is there any reason not to use "%4" in the definition?

define i32 @f(i32, i32) {
  %3 = add i32 %0, %1
  br label %4

%4:
  ret i32 %3
}

Maybe it creates an ambiguity in the grammar or something.

+1. Is there any reason not to use "%4" in the definition?

define i32 @f(i32, i32) {
  %3 = add i32 %0, %1
  br label %4

%4:
  ret i32 %3
}

Maybe it creates an ambiguity in the grammar or something.

A % isn't used for labels that do have names, so David's approach seems consistent.

+1. Is there any reason not to use "%4" in the definition?

define i32 @f(i32, i32) {
  %3 = add i32 %0, %1
  br label %4

%4:
  ret i32 %3
}

Maybe it creates an ambiguity in the grammar or something.

A % isn't used for labels that do have names, so David's approach seems consistent.

s/David/James/

jyknight updated this revision to Diff 187963.Feb 22 2019, 12:11 PM

Add some wording to LangRef and clang-format.

I like this idea, and I don’t think the textual IR central is too important. A few things:

  • Changes to the IR should always be discussed on llvm-dev. Did this already happen?

I sent a message ("Improving textual IR format for nameless blocks") concurrently with sending this review, and will wait a bit to make sure there's no objections.

  • Please update LangRef.

Done.

  • Did you write a script for updating the test cases? If so, you might consider attaching it to the RFC and linking to it from the commit message as a courtesy to downstream maintainers.

Nope, I didn't; there were few enough instances that it didn't seem worth scripting.

I have a few nitpicks, but otherwise this seems right. I'll wait for the llvm-dev discussion before LGTM'ing though.

llvm/lib/AsmParser/LLParser.cpp
2930–2931 ↗(On Diff #187963)

Since the label doesn't include a %, I think you should drop that part of the error message.

2936–2937 ↗(On Diff #187963)

I think single quotes around the number would be better here.

5580–5581 ↗(On Diff #187963)

Looks unrelated to this patch.

jyknight updated this revision to Diff 187994.Feb 22 2019, 2:50 PM
jyknight marked 4 inline comments as done.

Minor tweaks per comments.

Sorry, forgot to re-ping this in a timely manner. :)

The discussion on mailing list concluded positively, so waiting for an LG here.

This revision is now accepted and ready to land.Mar 18 2019, 3:25 PM
This revision was automatically updated to reflect the committed changes.