This is an archive of the discontinued LLVM Phabricator instance.

Keep attributes, calling convention, etc, when remangling intrinsic
ClosedPublic

Authored by dberlin on Feb 27 2017, 11:59 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

dberlin created this revision.Feb 27 2017, 11:59 AM
sanjoy added inline comments.Feb 27 2017, 12:05 PM
lib/IR/AutoUpgrade.cpp
1924 ↗(On Diff #89915)

I think we also need to undo the name change. Setting it and then unsetting it seems a bit hacky, perhaps we could have the switch create an unnamed NewCI, and outside it do:

if (NewCI != CI) {
  // Adjust names, and kill CI
}

(and remove the pre-rename)?

dberlin added inline comments.Feb 27 2017, 12:42 PM
lib/IR/AutoUpgrade.cpp
1924 ↗(On Diff #89915)

Just out of curiosity, why?
I mean, it renames it to old, but who cares?

It is just the name of the call instruction, not the actual called function?

IE it's just changing

%1 = call foo
into
%1.old = call foo

We could call it ".upgraded" instead?
:)

Actually, looking closer, all the other paths erase CI completely or leave it alone, so i'm not sure why both changing the name at all.
It looks like we are just wasting time?

IE all the paths before it depend on !NewFn and then destroy it, even if there is no replacement.
So if we get to the renaming, we've done nothing to it.

All the paths after either do nothing, erase CI completely, or are the one we are talking about.

Thus it seems we could just leave the name alone and declare victory?

Or am i missing something?

sanjoy added inline comments.Feb 27 2017, 12:50 PM
lib/IR/AutoUpgrade.cpp
1924 ↗(On Diff #89915)

I believe that if you call setName on the new instruction without changing the name of the old one, since two instructions with the same name are not allowed, the name it actually gets is name.0 or something like that (see ValueSymbolTable::createValueName). I think renaming the old instruction is a trick to prevent this from happening by avoiding the name collision.

I personally do not think it is important to avoid names like name.0 when auto-upgrading; but it looks like it was important enough for someone to add that code (assuming I read the intent correctly).

dberlin marked 3 inline comments as done.Feb 27 2017, 6:25 PM
dberlin added inline comments.
lib/IR/AutoUpgrade.cpp
1924 ↗(On Diff #89915)

So, it looks like the code used to rename it to .old in exactly one case.

In that case, it also used a dangling stringref (it's now a std::string copy, as you can see).

As a fix, the renaming code got hoisted, and now everything is renamed
anyway, i did what you suggested in the first case

dberlin marked an inline comment as done.
  • Update for comments
sanjoy added inline comments.Feb 27 2017, 9:50 PM
lib/IR/AutoUpgrade.cpp
1915 ↗(On Diff #89960)

How about NewCall instead of Repl?

2008 ↗(On Diff #89960)

These should just be Repl = Builder.CreateCall(NewFn, {BC0, BC1}, Name);

2048 ↗(On Diff #89960)

I'm not sure if this does what used to happen before. To get the behavior intended by https://reviews.llvm.org/rL146357 I think CI->setName(Name + ".old"); needs to happen before you create a new call instruction with Name as name. Either that, or some shuffling as:

if (Name not empty) {
  CI->setName(Name + ".old");
  Repl->setName(Name); // This one should "stick"
  ...
}
dberlin added inline comments.Feb 28 2017, 5:05 AM
lib/IR/AutoUpgrade.cpp
2008 ↗(On Diff #89960)

Yeah, and the break above should be a return.
Looks like my refactoring tool screwed it up.

2048 ↗(On Diff #89960)

I don't see a way to set the name beforehand, except either unconditionally, or to do so inside the switches.
The second we could do.
Do we really still believe this is all worth it?

Your original suggestion was:

"if (NewCI != CI) {

// Adjust names, and kill CI

}
(and remove the pre-rename)?"

That won't work here, as discussed :)
Do we think this is all better than just removing the renaming and letting it all work itself out?
(or always renaming)

dberlin marked an inline comment as done.Feb 28 2017, 8:05 AM
  • Update for comments
sanjoy added inline comments.Feb 28 2017, 11:00 AM
lib/IR/AutoUpgrade.cpp
1920 ↗(On Diff #90037)

Don't you need a break; here?

1953 ↗(On Diff #90037)

Spacing is a bit off here.

1958 ↗(On Diff #90037)

Why did you leave the replaceAllUsesWith alone in this case?

dberlin marked 3 inline comments as done.Feb 28 2017, 11:05 AM
dberlin added inline comments.
lib/IR/AutoUpgrade.cpp
1920 ↗(On Diff #90037)

No, this should return, sigh.

1953 ↗(On Diff #90037)

I'll clang format it again

1958 ↗(On Diff #90037)

Just got missed because it was multiline

sanjoy added inline comments.Feb 28 2017, 11:08 AM
lib/IR/AutoUpgrade.cpp
1920 ↗(On Diff #90037)

If you change this to a return then how about keeping NewCall uninitialized (instead of setting it to CI) above and removing the NewCall != CI check below?

dberlin marked 5 inline comments as done.
  • Update for comments
sanjoy accepted this revision.Feb 28 2017, 3:18 PM

lgtm with two minor comments inline.

lib/IR/AutoUpgrade.cpp
1955 ↗(On Diff #90089)

We're doing redundant work here -- I don't think we need to pass in Name to CreateCall, since we're going to set the name explicitly anyway.

Once we've removed these uses of Name, we should be able to sink the std::string Name = CI->getName(); to after the switch.

test/CodeGen/Generic/overloaded-intrinsic-name.ll
2 ↗(On Diff #90089)

Do we really need a new RUN line here? Why not just tack on a | FileCheck %s to the already existing line?

This revision is now accepted and ready to land.Feb 28 2017, 3:18 PM
This revision was automatically updated to reflect the committed changes.