Page MenuHomePhabricator

IR: accept and print numbered %N names for function args
ClosedPublic

Authored by t.p.northover on Aug 1 2019, 8:28 AM.

Details

Summary

Currently unnamed function arguments get assigned the first available numbers %0, %1, ... and developers are just expected to know that and count arguments if needed. It would be better to print out the numbers explicitly, like we do for instructions in the body.

It's worth noting that updating front-ends' tests will probably be a bit of work, but I've got a (embarrassingly bad) script that gets about 80-90% of the way there. I'll reply to any commit with it attached.

Diff Detail

Event Timeline

t.p.northover created this revision.Aug 1 2019, 8:28 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

+1 for doing this. I started looking at fixing this when I modified the printer to print proper labels for numbered basic-blocks (instead of comments), but I didn't do so because of the amount of test churn was off-putting.

I think that after this change, there's only one local entity left which uses a local-value-number but doesn't print it: the entry block. That also would cause a quite-large amount of test-churn to add.

llvm/lib/IR/AsmWriter.cpp
3561

I think you need a space before this string? Although, then shouldn't llvm/unittests/IR/AsmWriterTest.cpp be failing? (it has a space there...)

jdoerfert resigned from this revision.Aug 1 2019, 9:58 AM

I like the idea but I am not the right person to review.

rnk added a comment.Aug 1 2019, 10:43 AM

This is awesome! Thanks for doing the test updates. As I read it, LLVM will still read old-style IR just fine, which seems nice.

I think it would be worth noting this in the 10.0 release notes, since it will affect someone upgrading a frontend as you mention.

Otherwise, I think this is basically good to go, unless I'm forgetting anything...

Macro celebration_balloons:

  • Added release note
  • Added hacky script to utils so that I can refer to it from the release note.
  • Removed "<badref>" handling from printArgument. Printing a free-standing Argument goes down a different path.
rnk accepted this revision.Aug 2 2019, 2:21 PM

lgtm

llvm/utils/add_argument_names.py
5

Nice.

This revision is now accepted and ready to land.Aug 2 2019, 2:21 PM
t.p.northover accepted this revision.Aug 3 2019, 7:28 AM
t.p.northover marked an inline comment as done.

Thanks Reid, committed as r367755.

llvm/utils/add_argument_names.py
5

Incoming prize for the most egregious violation of the 70 column rule.

t.p.northover closed this revision.Aug 3 2019, 7:28 AM