[llvm-commits] FW: [llvm] [patch] Porting Hexagon MI Scheduler to new API

[Ron Lieberman]

Hi Sergei
I reviewed it as well, and have no issues.

Ron Lieberman
---
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation

-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu
[mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Sergei Larin
Sent: Wednesday, August 29, 2012 3:47 PM
To: 'Sebastian Pop'
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] FW: [llvm] [patch] Porting Hexagon MI Scheduler
to new API

Sebastian,

  Thank you for the review. Here is updated patch. If no one else has any
comments, may I assume it is OK to commit?

Thanks.

Sergei

---
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation

> -----Original Message-----
> From: Sebastian Pop [mailto:spop at codeaurora.org]
> Sent: Wednesday, August 29, 2012 9:17 AM
> To: Sergei Larin
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] FW: [llvm] [patch] Porting Hexagon MI 
> Scheduler to new API
> 
> Hi Sergei,
> 
> > Index: lib/Target/Hexagon/HexagonMachineScheduler.cpp
> > ===================================================================
> > --- lib/Target/Hexagon/HexagonMachineScheduler.cpp	(revision 0)
> > +++ lib/Target/Hexagon/HexagonMachineScheduler.cpp	(revision 0)
> > @@ -0,0 +1,871 @@
> > +//===- HexagonMachineScheduler.cpp - MI Scheduler for Hexagon 
> > +-------------===//
> > +//===--------------------------------------------------------------
> > +-
> -
> > +------===//
> > +//
> > +// MachineScheduler schedules machine instructions after phi 
> > +elimination. It // preserves LiveIntervals so it can be invoked
> before register allocation.
> > +//
> > +//===--------------------------------------------------------------
> > +-
> -
> > +------===//
> 
> I see that you are adding this new file without the UIUC license 
> notice at the beginning.  Please add this license notice to all new files:
> 
> //                     The LLVM Compiler Infrastructure
> //
> // This file is distributed under the University of Illinois Open 
> Source // License. See LICENSE.TXT for details.
> 
> > Property changes on: lib/Target/Hexagon/HexagonMachineScheduler.h
> > ___________________________________________________________________
> > Added: svn:executable
> >    + *
> >
> [...]
> > Property changes on: lib/Target/Hexagon/HexagonMachineScheduler.cpp
> > ___________________________________________________________________
> > Added: svn:executable
> >    + *
> 
> You shouldn't chmod +x these files: please chmod 644 like any other 
> file in the Hexagon backend.
> 
> 
> > Index: lib/Target/Hexagon/HexagonTargetMachine.cpp
> [...]
> > @@ -69,6 +82,7 @@
> >  // Independent Optimization passes to the Pass Manager.
> >  bool 
> > HexagonTargetMachine::addPassesForOptimizations(PassManagerBase
> > &PM) {
> >
> > +
> >    PM.add(createConstantPropagationPass());
> >    PM.add(createLoopSimplifyPass());
> >    PM.add(createDeadCodeEliminationPass());
> >
> [...]
> > Index: lib/Target/Hexagon/HexagonRegisterInfo.cpp
> > ===================================================================
> > --- lib/Target/Hexagon/HexagonRegisterInfo.cpp	(revision 162202)
> > +++ lib/Target/Hexagon/HexagonRegisterInfo.cpp	(working copy)
> > @@ -44,6 +44,7 @@
> >     TII(tii) {
> >  }
> >
> > +
> >  const uint16_t* HexagonRegisterInfo::getCalleeSavedRegs(const
> MachineFunction
> >                                                          *MF)
> >    const {
> > @@ -163,6 +164,7 @@
> >
> >    const unsigned FrameSize = MFI.getStackSize();
> >
> > +
> >    if (!MFI.hasVarSizedObjects() &&
> >        TII.isValidOffset(MI.getOpcode(), (FrameSize+Offset)) &&
> >        !TII.isSpillPredRegOp(&MI)) { @@ -192,6 +194,7 @@
> >            getSubReg(MI.getOperand(0).getReg(),
> Hexagon::subreg_loreg) :
> >            MI.getOperand(0).getReg();
> >
> > +
> >          // Check if offset can fit in addi.
> >          if (!TII.isValidOffset(Hexagon::ADD_ri, Offset)) {
> >            BuildMI(*MI.getParent(), II, MI.getDebugLoc(),
> >
> 
> Please remove these changes.
> 
> > +/// (ScoreboardHazardRecognizer) and arbitraty target-specific
> logic.
> 
> s/arbitraty/arbitrary/
> 
> > +  }
> > +  else if (ForceBottomUp) {
> [...]
> > +  }
> > +  else {
> 
> These should stand on a single line.
> 
> In general, the patch looks great.  Please address these typos, and 
> you'll be ready to go.
> 
> Thanks,
> Sebastian
> --
> Qualcomm Innovation Center, Inc is a member of Code Aurora Forum