This is an archive of the discontinued LLVM Phabricator instance.

[MIR] Print target specific constant pools
ClosedPublic

Authored by rovka on Jul 31 2017, 8:07 AM.

Details

Summary

This should enable us to test the generation of target-specific constant
pools, e.g. for ARM:

constants:

  • id: 0 value: 'g(GOT_PREL)-(LPC0+8-.)' alignment: 4 isTargetSpecific: true

I intend to use this to test PIC support in GlobalISel for ARM.

I don't really know how to test it outside of that context, since the
existing MIR tests usually rely on parser support as well, and that
seems a bit trickier to add. I don't have an immediate use for that, but
I can put in the extra effort if people point me in the right direction
with it (i.e. where would we put the target-specific parsing?
Piggy-back it on the Subtarget, maybe?). Alternatively, I could try to
add a unit test, but the setup for that seems rather convoluted and I'm
not sure it's the best way to go about this.

Thoughts?

Diff Detail

Event Timeline

rovka created this revision.Jul 31 2017, 8:07 AM
arphaman edited edge metadata.Jul 31 2017, 9:22 AM

It might not be worth doing full blown target-specific serialization for this, but it might be nice to add IsTargetSpecific optional field to the constant pool value YAML representation. This way the parser will at least know that it can't parse this kind of constant pool value when somebody tries to feed the printed MIR into llc.

The last time I thought about the target-specific serialization I had something like this in mind:
I think for target specific serialization it might make sense to inject a set of values into the YAML mapping. The parser would have to call into a target hook that could create a target-specific ConstantPoolValues from the YAML mapping (ARM has multiple subclasses, so you'd have to somehow store that information as well which might be an issue, although you could probably figure out the subclass just by looking at the keys in the YAML mapping). But again, I'm not sure it's worth it though, since ARM would have to provide YAML mappings for each individual enum that's used in constant pool values and the subclasses of ConstantPoolValue. Another problem is the MachineBasicBlock/Global value references, which are parsed after YAML is deserialized. ARM's parser might have to manually parse these (call into the parseMBBReference function`) which could be annoying. I think that could be partially solved using new types instead of raw strings the YAML traits. These types should expose only a function that can produce a real value (like MBB*), and not the raw string.

Another way to handle this would be to just print the value (as you did here) and provide a target-specific parsing hook and then the targets can just parse the printed value if they want to parse MIR with their target-specific values. But I think that's a bit crude and doesn't integrate with the rest of the MIR that well.

rovka updated this revision to Diff 109081.Aug 1 2017, 4:31 AM
rovka edited the summary of this revision. (Show Details)

Thanks for the detailed response!

I added another field (isTargetSpecific) and made the parser produce a more meaningful error message when encountering one of those.

I'll look a bit more into the parser, but I'm not making any promises yet :)

Sure thing, that can probably be done in another patch though.

The change LGTM. However, it lacks a test that verifies that the parser reports an error.

rovka updated this revision to Diff 109311.Aug 2 2017, 3:04 AM

Refresh before commit. Now including a test for the error-path!

This revision was automatically updated to reflect the committed changes.