Page MenuHomePhabricator

IR: Introduce local_unnamed_addr attribute.
AbandonedPublic

Authored by pcc on May 13 2016, 2:50 PM.

Details

Reviewers
tstellarAMD

Diff Detail

Event Timeline

pcc updated this revision to Diff 57258.May 13 2016, 2:50 PM
pcc retitled this revision from to [wip] IR: Introduce local_unnamed_addr attribute..
pcc updated this object.
rafael edited edge metadata.May 16 2016, 9:11 AM
rafael added a subscriber: llvm-commits.
rafael added a subscriber: rafael.

I much prefer this version.

lib/CodeGen/Analysis.cpp
626

We should probably either disallow unnamed_addr on variables or says that whoever put it there knows what he is doing.

For now I would suggest just leaving this part here.

majnemer added inline comments.
include/llvm/IR/GlobalValue.h
162–164

I think it'd be more clear what this does if it were renamed hasGlobalUnnamedAddr. It's current name implies that UnnamedAddr != NotUnnamedAddr instead of UnnamedAddr == GlobalUnnamedAddr.

171–173

I'd expect this to be UnnamedAddr == LocalUnnamedAddr. I'd either choose a better name for this function or change the callers to check for both UnnamedAddrType values.

majnemer added inline comments.May 16 2016, 9:38 AM
include/llvm/IR/GlobalValue.h
156–160

NotUnnamedAddr is a little awkward.

Perhaps it'd be a little easier if we made UnnamedAddrType an enum class.
Something like:

enum class UnnamedAddr {
  None,
  Local,
  Global,
};
pcc added a comment.May 17 2016, 1:50 PM

Should the verifier check if this the property is invalidly set?

I discussed this a little with @majnemer on IRC. I think we cannot do this because passes should be allowed to introduce benign address comparisons. One example of this is speculative comparison of vtable addresses for devirtualization (vtables are unnamed_addr in C++, but imagine some other language where they weren't). If globalopt were to mark globals local_unnamed_addr, that would cause any speculative comparison to fail to verify.

One way to solve the problem could be to teach the devirtualizer to clear the local_unnamed_addr attribute, but imagine some case where some pass that runs after the devirtualizer uncovers the comparison (for example, if a constructor is inlined into a devirtualized call site, that might cause both sides of the comparison to become constant). This means that any number of passes would need to protect against this situation.

pcc updated this revision to Diff 57519.May 17 2016, 2:28 PM
pcc marked 4 inline comments as done.
  • Address review comments
  • Add documentation to BitCodeFormat and LangRef
pcc retitled this revision from [wip] IR: Introduce local_unnamed_addr attribute. to IR: Introduce local_unnamed_addr attribute..May 17 2016, 2:28 PM
pcc updated this revision to Diff 57526.May 17 2016, 2:53 PM
  • Minimise the diff a little
  • Add assembler/bitcode test

You might want to add a case to compatibility.ll too.

docs/BitCodeFormat.rst
727 ↗(On Diff #57526)

What are the white space changes? Can you commit them first if it is a cleanup?

836 ↗(On Diff #57526)

Can you commit these first? (they LGTM).

lib/IR/AsmWriter.cpp
2427

This might look nicer with a function that returns "", "local_unamed_addr " or "unnamed_addr ".

2663

That way you get use the helper in here too.

pcc updated this revision to Diff 57536.May 17 2016, 4:07 PM
  • Add another use of helper function
  • Update compatibility.ll
pcc marked 2 inline comments as done.May 17 2016, 4:08 PM

You might want to add a case to compatibility.ll too.

Done.

docs/BitCodeFormat.rst
727 ↗(On Diff #57526)

Yes, r269857.

836 ↗(On Diff #57526)

As above

Folks, llvm-commits wasn't even on the original version of this patch, and there is essentially no high-level description of the problem being solved or the motivation of the patch.

Can you start a fresh revision in phab and actually include a *lot* more context? I suspect many, many people will be very interested in something as impactful as a new variant of 'unnamed_addr' and they may (like me) have completely missed the discussion taking place here or lack significant context for it.

pcc added a comment.May 17 2016, 4:57 PM

Hi Chandler, the initial patch/discussion/context was on D20258 which did start on the mailing list. Admittedly some of that didn't make it here, so I'll create a fresh revision with a better commit message.

Seriously I absolutely don't get the "start a new revision" thing because the initial email was not on llvm-commit. Really what is the rational?

We are losing the history and the threading for the review, this is really annoying.

So big -1

pcc abandoned this revision.May 17 2016, 5:59 PM

Created D20348