This is an archive of the discontinued LLVM Phabricator instance.

Describe stack-id as an enum
ClosedPublic

Authored by sdesmalen on Apr 2 2019, 9:26 AM.

Details

Summary

This patch changes MIR stack-id from an integer to an enum,
and adds printing/parsing support for this in MIR files. The default
stack-id '0' is now renamed to 'default'.

This should make MIR tests that have stack objects with different stack-ids
more descriptive. It also clarifies code operating on StackID.

Diff Detail

Event Timeline

sdesmalen created this revision.Apr 2 2019, 9:26 AM
arsenm added inline comments.Apr 2 2019, 9:34 AM
include/llvm/CodeGen/MIRYamlMapping.h
316

I think using - instead of _ follows the current naming conventions.

I'm not sure a target specific usage should be put here, but maybe

This is definitely nicer, thanks for working on it.

I have two concerns about this though:

  • we're going from a target-specific "namespace" to a target-independent one, which means in this case that no other target can use the stack id 1
  • we're exposing target-specific names and enums in the target-independent code

This is definitely nicer, thanks for working on it.

I have two concerns about this though:

  • we're going from a target-specific "namespace" to a target-independent one, which means in this case that no other target can use the stack id 1
  • we're exposing target-specific names and enums in the target-independent code

Thanks! Yes, you're right that exposing the target-specific names/enums in target-independent code is less desirable, although note that this happens in some other places in LLVM as well; think for example of the enum in CallingConv. I made the trade-off to use the enum for several reasons:

  • By using an enum way we don't get target-specific MIR parsing of common properties.
  • If we want to make it target-independent, we'll need to manually parse it as a string and we lose out on out-of-the box YAML parsing, printing, verification and diagnostics of having an enum value.
  • If the value needs to be a string, it will have ugly quotes around it :)
  • At the moment there seems to be only very limited use of stack-id, in trunk only the AMDGPU target as far as I know. Unless we care deeply about allowing to programatically generate a range of stack-ids at compile-time (with their meaning only known at compile-time), I don't think it is much of a limitation to have a single namespace.
sdesmalen marked an inline comment as done.Apr 3 2019, 4:55 AM
sdesmalen added inline comments.
include/llvm/CodeGen/MIRYamlMapping.h
316

Good point, I'll make sure the next iteration of the patch uses '-' instead!

sdesmalen updated this revision to Diff 193730.Apr 4 2019, 8:47 AM
  • Renamed sgpr_spill -> sgpr-spill.
  • Renamed TargetStackID::SGPR_Spill -> TargetStackID::SGPRSpill.
arsenm accepted this revision.Jun 14 2019, 7:28 AM

LGTM

This revision is now accepted and ready to land.Jun 14 2019, 7:28 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 2:11 AM
Herald added a subscriber: jrtc27. · View Herald Transcript