Page MenuHomePhabricator

IR, X86: Understand !range metadata on global variables.
ClosedPublic

Authored by pcc on Oct 21 2016, 12:47 PM.

Details

Summary

Attaching !range to a global variable does two things:

  1. Marks it as an absolute symbol reference.
  2. Specifies the value range of that symbol's address.

Teach the X86 backend to allow absolute symbols to appear in place of
immediates by extending the relocImm and mov64imm32 matchers. Start using
relocImm in more places where it is legal.

As previously proposed on llvm-dev:
http://lists.llvm.org/pipermail/llvm-dev/2016-October/105800.html

Depends on D25812

Depends on D25862

Depends on D25877

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 75465.Oct 21 2016, 12:47 PM
pcc retitled this revision from to IR, X86: Understand !range metadata on global variables..
pcc updated this object.
pcc added reviewers: chandlerc, lattner.
pcc added a subscriber: llvm-commits.

The IR representation you're using in your testcases is abusing the meaning of a global variable. You're declaring "variables" whose address doesn't actually correspond to an object in memory. In addition to being ugly, this could have nasty implications for alias analysis: we assume that global variables don't alias each other. (See llvm::isIdentifiedObject etc.)

Some sort of GlobalConstant might make sense if the code patterns from your testcases are useful.

pcc added a reviewer: sanjoy.Oct 21 2016, 3:41 PM

The IR representation you're using in your testcases is abusing the meaning of a global variable.

Yes, I think this is certainly one of the downsides of this representation.

we assume that global variables don't alias each other. (See llvm::isIdentifiedObject etc.)

Orthogonally: this assumption doesn't seem sound in general. An object file may define multiple external symbols with the same address (e.g. using GlobalAlias) which may be referenced using separate external GlobalVariables.

Some sort of GlobalConstant might make sense if the code patterns from your testcases are useful.

Maybe. I'm not sure how feasible that approach would be though. I'll experiment with it a little and see.

lattner edited edge metadata.Oct 25 2016, 10:12 PM

I think there are two things going on here. I think that eli is right: LLVM IR global variables should always be assumable that they don't alias. This is guaranteed by C and other source languages and is important for LLVM to be able to represent. Separately though, I think it makes sense for LLVM globals to have !range metadata to specify that (though disjoint) they live in a specific address range that can fit into immediates.

There is a degenerate case where a global has a known address (either through magic knowledge by the frontend, or by a clang attribute, or by the compiler somehow knowing about a linker map in flight, e.g. with LTO) can be either represented with a specific !range metadata or with a new GlobalConstant address. I think it would be better to discuss such significant IR changes on llvmdev though.

pcc updated this revision to Diff 77224.Nov 8 2016, 11:42 AM
pcc edited edge metadata.
  • Update LangRef
  • May not assume that absolute symbol refs are non-null

As discussed on the llvm-dev thread, we're going with the approach in this patch. I've updated the langref and added a nonnull check analogous to what I did in D25930.

Adding X86 reviewers to take a look at the X86-specific bits of this.

craig.topper added inline comments.Nov 8 2016, 10:59 PM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1725 ↗(On Diff #77224)

This code WasTruncated stuff could probably use some comments explaining it.

1735 ↗(On Diff #77224)

Were the 7 different opcode checks we had before significant? It safe to replace them with this single check for TargetGlobalAddress?

RKSimon resigned from this revision.Nov 12 2016, 8:52 AM
RKSimon removed a reviewer: RKSimon.
pcc updated this revision to Diff 78129.Nov 15 2016, 7:24 PM
  • Add more comments
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1725 ↗(On Diff #77224)

Added more comments here and in the rest of this function.

1735 ↗(On Diff #77224)

I looked at all places where we create X86ISD::Wrapper nodes by following links from http://llvm-cs.pcc.me.uk/lib/Target/X86/X86ISelLowering.h/rWrapper

My conclusion was that I believe that it is only possible for us to have these 7 nodes as operands of X86ISD::Wrapper.

We can make this simplification separately; I've sent out D26731.

pcc updated this revision to Diff 78268.Nov 16 2016, 2:48 PM

Refresh

efriedma added inline comments.Dec 6 2016, 3:31 PM
llvm/docs/LangRef.rst
4579 ↗(On Diff #78268)

This is sort of nitpicking, but could we call this something other than !range? Maybe !absolute_symbol? It's very different from the other uses of range metadata, given that it has mandatory effects on code generation.

pcc added inline comments.Dec 6 2016, 4:02 PM
llvm/docs/LangRef.rst
4579 ↗(On Diff #78268)

Seems reasonable, will do.

pcc updated this revision to Diff 80502.Dec 6 2016, 4:25 PM
pcc marked an inline comment as done.
  • Rename metadata to absolute_symbol
pcc added a comment.Dec 6 2016, 4:26 PM

(I'll s/range/absolute_symbol/g in the commit message when I commit, don't want to break the thread by changing it now.)

The IR changes look fine; someone else should probably review the backend changes.

pcc added a comment.Dec 6 2016, 4:41 PM

Thanks Eli. @craig.topper do the backend changes look good now?

craig.topper edited edge metadata.Dec 7 2016, 11:00 PM

Backend changes LGTM.

This revision was automatically updated to reflect the committed changes.