This is an archive of the discontinued LLVM Phabricator instance.

Move Post RA Scheduling flag bit into SchedMachineModel
ClosedPublic

Authored by spatel on Jun 19 2014, 9:36 AM.

Details

Summary

Currently, there's an "X86ProcFamily" enum that is used to enable post-register-allocator code scheduling for Atom CPU variants. It would be better to explicitly enable/disable this scheduler feature using a flag in the subtarget's SchedMachineModel rather than a bit in the X86Subtarget class.

This minimal patch just moves the PostRAScheduler flag bit and fixes up the accesses to that bit. Subsequent patches will clean up the X86ProcFamilyEnum. Other targets (ARM also has a PostRAScheduler bit in ARMSubtarget.h) may want to make a similar change to keep uniformity, or maybe it's better to move this bit into the parent TargetSubtargetInfo?

No change to codegen for any target is intended with this patch.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 10640.Jun 19 2014, 9:36 AM
spatel retitled this revision from to Move Post RA Scheduling flag bit into SchedMachineModel.
spatel updated this object.
spatel edited the test plan for this revision. (Show Details)
spatel added reviewers: hfinkel, nadav.
spatel added a subscriber: Unknown Object (MLST).
hfinkel added inline comments.Jun 24 2014, 1:50 PM
include/llvm/Target/TargetSchedule.td
91 ↗(On Diff #10640)

This needs a comment.

lib/Target/X86/X86Subtarget.cpp
364 ↗(On Diff #10640)

I'd like to see the logic moved into PostRAScheduler::runOnMachineFunction (so that every target does not have to explicitly check the scheduler model) along with the call to this enablePostRAScheduler function and the corresponding command-line-argument check. The interface may not really be setup correctly for this currently, but we can change it.

atrick accepted this revision.Jun 28 2014, 10:50 PM
atrick added a reviewer: atrick.
atrick added a subscriber: atrick.

LGTM

This revision is now accepted and ready to land.Jun 28 2014, 10:50 PM
spatel updated this revision to Diff 11068.Jul 3 2014, 4:39 PM
spatel edited edge metadata.

Partial implementation of moving enablePostRAScheduler() into the PostRAScheduler class. We now need virtual functions for the AntiDepBreakMode and CriticalPathRCs - and I think we'll need another for the OptLevel too. I didn't implement any of those for any targets in case I've misunderstood the request.

Does this match what you are suggesting, Hal?

hfinkel edited edge metadata.Jul 8 2014, 9:36 AM

LGTM too. Please separate the X86 functional changes from the generic infrastructure changes into distinct commits.

spatel added a comment.Jul 8 2014, 1:28 PM

Hal - let me make sure we're on the same page. The last patch here is only a partial implementation of moving enablePostRAScheduler().

I will need to add methods to all targets that use enablePostRAScheduler() to override the defaults for:
getCriticalPathRCs()
getAntiDepBreakMode()
getOptLevelToEnablePostRAScheduler() -- this one isn't in the patch yet even in the TargetSubtargetInfo superclass, but it's necessary

I can't do this in separate commits without breaking the existing behavior of targets that override enablePostRAScheduler(). All of the those targets will need to implement their custom versions of the 3 virtual methods to maintain the current settings.

  • Original Message -----

From: "Sanjay Patel" <spatel@rotateright.com>
To: spatel@rotateright.com, nrotem@apple.com, atrick@apple.com, hfinkel@anl.gov
Cc: "amara emerson" <amara.emerson@arm.com>, llvm-commits@cs.uiuc.edu
Sent: Tuesday, July 8, 2014 3:28:18 PM
Subject: Re: [PATCH] Move Post RA Scheduling flag bit into SchedMachineModel

Hal - let me make sure we're on the same page. The last patch here is
only a partial implementation of moving enablePostRAScheduler().

I will need to add methods to all targets that use
enablePostRAScheduler() to override the defaults for:
getCriticalPathRCs()
getAntiDepBreakMode()
getOptLevelToEnablePostRAScheduler() -- this one isn't in the patch
yet even in the TargetSubtargetInfo superclass, but it's necessary

I can't do this in separate commits without breaking the existing
behavior of targets that override enablePostRAScheduler(). All of
the those targets will need to implement their custom versions of
the 3 virtual methods to maintain the current settings.

Right, this makes sense. Please proceed with this work.

You're right, you can't separate the X86 changes because they keep the current behavior. Please ignore that comment of mine ;)

Thanks again,
Hal

http://reviews.llvm.org/D4217

spatel updated this revision to Diff 11224.Jul 9 2014, 3:49 PM
spatel edited edge metadata.

Completed patch to update all targets to use the new interface. This is purely a refactoring job; no functional changes are intended.

Summary:

  1. Removed PostRAScheduler bits from subtargets (X86, ARM).
  2. Added PostRAScheduler bit to MCSchedModel class.
  3. This bit is set by a CPU's scheduling model (if it exists).
  4. Removed enablePostRAScheduler() function from TargetSubtargetInfo and subclasses.
  5. Fixed the existing enablePostMachineScheduler() method to use the MCSchedModel (was just returning false!).
  6. Added methods to TargetSubtargetInfo to allow overrides for AntiDepBreakMode, CriticalPathRCs, and OptLevel for PostRAScheduling.
  7. Added enablePostRAScheduler() function to PostRAScheduler class which queries the subtarget for the above values.
  8. Preserved existing scheduler behavior for ARM, MIPS, PPC, and X86: a. ARM overrides the CPU's postRA settings by enabling postRA for any non-Thumb or Thumb2 subtarget. b. MIPS overrides the CPU's postRA settings by enabling postRA for everything. c. PPC overrides the CPU's postRA settings by enabling postRA for everything. d. X86 is the only target that actually has postRA specified via sched model info.

Not sure what the policy is on this - Hal, you already marked this with 'LGTM', but the patch has grown quite a bit since that. Do you want to give this another look over?

  • Original Message -----

From: "Sanjay Patel" <spatel@rotateright.com>
To: spatel@rotateright.com, nrotem@apple.com, atrick@apple.com, hfinkel@anl.gov
Cc: "amara emerson" <amara.emerson@arm.com>, llvm-commits@cs.uiuc.edu
Sent: Sunday, July 13, 2014 11:15:46 AM
Subject: Re: [PATCH] Move Post RA Scheduling flag bit into SchedMachineModel

Not sure what the policy is on this - Hal, you already marked this
with 'LGTM', but the patch has grown quite a bit since that.

Generally speaking, we ask contributors to use their best judgment as to whether the additional changes are obvious/mechanical-enough or not.

Do you
want to give this another look over?

Sure. I'll look at it again.

-Hal

http://reviews.llvm.org/D4217

LGTM, again.

spatel closed this revision.Jul 15 2014, 3:48 PM
spatel updated this revision to Diff 11474.

Closed by commit rL213101 (authored by @spatel).