[llvm-dev] RFC: Absolute or "fixed address" symbols as immediate operands

Tue Oct 11 20:13:18 PDT 2016

On Tue, Oct 11, 2016 at 3:15 PM, Peter Collingbourne <peter at pcc.me.uk>
wrote:

>
>
> On Tue, Oct 11, 2016 at 2:48 PM, Chris Lattner <clattner at apple.com> wrote:
>
>> On Oct 11, 2016, at 12:04 AM, Peter Collingbourne <peter at pcc.me.uk>
>> wrote:
>>
>> I have been experimenting with a number of approaches to representation
>>> in SDAG, and I have found one that seems to work best, and would be the
>>> least intrusive (unfortunately most approaches to this problem are somewhat
>>> intrusive).
>>>
>>> Specifically, I want to:
>>> 1) move most of the body of ConstantSDNode to a new class,
>>> ConstantIntSDNode, which would derives from ConstantSDNode. ConstantSDNode
>>> would act as the base class for immediates-post-static-linking. Change
>>> most references to ConstantSDNode in C++ code to refer to
>>> ConstantIntSDNode. However, "imm" in tblgen code would continue to match
>>> ConstantSDNode.
>>> 2) introduce a new derived class of ConstantSDNode for references to
>>> globals with !range metadata, and teach SDAG to use this new derived class
>>> for fixed address references
>>>
>>>
>>> ConstantSDNode is poorly named, and renaming it to ConstantIntSDNode is
>>> probably the right thing to do independently of the other changes.
>>>
>>> That said, I don’t understand why you’d keep ConstantSDNode around and
>>> introduce a new derived class of it.  This seems like something that a new
>>> “imm" immediate matcher would handle: it would match constants in a certain
>>> range, or a GlobalAddressSDNode known-to-be-small.
>>>
>>
>> To begin with: I'm not sure that GlobalAddressSDNode is the right node to
>> use for these types of immediates. It seems that we have two broad classes
>> of globals here: those with a fixed-at-link-time address (e.g. regular
>> non-PIC symbols, absolute symbols) and those where the address needs to be
>> computed (e.g. PC-relative addresses, TLS variables). To me it seems like
>> the first class is much more similar to immediates than to the second
>> class. That suggested to me that there ought to be two separate
>> representations for global variables, where the former are "morally"
>> immediates, and the latter are not (i.e. the existing GlobalAddressSDNode).
>>
>>
>> I understand what you’re saying, but I don’t think that is the key issue
>> here.  The relevant SDNode subclasses are concerned with representing the
>> structural input code (in this case a GlobalValue*) not about representing
>> the target-specific concept at work here (this particular GV has an address
>> known to fit in this specific relocation).  The structure of SelectionDAG
>> types like SDNode needs to be target independent, and target specific
>> matchers are the ones that handle discrepancies.
>>
>>
>> I went over a couple of approaches for representing "moral" immediates in
>> my llvm-commits post. The first one seems to be more like what you're
>> suggesting:
>>
>> > - Introduce a new opcode for absolute symbol constants.
>>
>>
>> If you mean a new ISD opcode, then I don’t think this makes sense.  We
>> already have an opcode for that represents the address of a global value,
>> we should use it.  “absolute symbol constants” are a special case of them,
>> and using a predicate to handle matching them should work fine.  What am I
>> missing?
>>
>> This intuitively seemed like the least risky approach, as individual
>> instructions could "opt in" to the new absolute symbol references. However,
>> this seems hard to fit into the existing SDAG pattern matching engine, as
>> the engine expects each "variable" to have a specific opcode. I tried
>> adding special support for "either of the two constant opcodes" to the
>> matcher, but I could not see a good way to do it without making fundamental
>> changes to how patterns are matched.
>>
>>
>> I think you’ll have to define the matcher in C++ with ComplexPattern,
>> analogously to how the addressing mode selection logic works.  This allows
>> you to specify multiple ISD nodes that it can match.
>>
>> > - Use the ISD::Constant opcode for absolute symbol constants, but
>> introduce a separate class for them. This also seemed problematic, as there
>> is a strong assumption (both in existing SDAG code and in generated code)
>> of a many-to-one mapping from opcodes to classes.
>>
>>
>> This also doesn’t make sense to me.  The fundamental issue you’re
>> grappling with is that you have two different “input” concepts (small
>> immediates, and globals whose absolute address fits in that range) that you
>> want to handle the same way.  You need to do something like ComplexPattern
>> to handle this.
>>
>
> Thanks Chris. I will take a closer look at ComplexPattern, I was somehow
> unaware of it until now. From a brief look it does seem to allow me to do
> what I need here while being less intrusive.
>
> I think my main concerns with it are that using ComplexPattern pervasively
> in instruction patterns may cause bloat in the pattern matching tables and
> that it may cause FastISel to bail out more often. Those don't seem like
> insurmountable problems though, we may just need a specialized variant of
> it.
>

I'm afraid I might be misunderstanding the basics here -- I tried to start
off by replacing an "imm" matcher in one of the x86 patterns with a
ComplexPattern that simply matches ConstantSDNodes, as shown in the
attached patch. I would have expected this to be a no-op change, but I
found that this change broke (i.e. caused assertion failures in) a large
number of test cases, e.g. "test/CodeGen/X86/2012-07-16-fp2ui-i1.ll".

Is there anything I'm doing wrong here, or are there just bugs that need to
be fixed?

Thanks,
-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161011/db7df421/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fancyimm.patch
Type: text/x-patch
Size: 2132 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161011/db7df421/attachment.bin>