[llvm] r180573 - This patch adds the X86FixupLEAs pass, which will reduce instruction

[Gurd, Preston]

Hello Nadav,

Thanks, once again, for your attentiveness!

I had added the documentation just ahead of each function body, using X86PadShortFunction.cpp and X86VZeroUpper.cpp as models.

I will move the comments for the functions in X86FixupLEAs.cpp to precede the initial declarations.

Preston

From: Nadav Rotem [mailto:nrotem at apple.com]
Sent: Thursday, April 25, 2013 4:51 PM
To: Gurd, Preston
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm] r180573 - This patch adds the X86FixupLEAs pass, which will reduce instruction

+
+    virtual const char *getPassName() const { return "X86 Atom LEA Fixup";}
+    void  seekLEAFixup(MachineOperand& p, MachineBasicBlock::iterator& I,
+                      MachineFunction::iterator MFI);
+    void processInstruction(MachineBasicBlock::iterator& I,
+                            MachineFunction::iterator MFI);
+    RegUsageState usesRegister(MachineOperand& p,
+                               MachineBasicBlock::iterator I);
+    MachineBasicBlock::iterator searchBackwards(MachineOperand& p,
+                                                MachineBasicBlock::iterator& I,
+                                                MachineFunction::iterator MFI);
+    MachineInstr* postRAConvertToLEA(MachineFunction::iterator &MFI,
+                                     MachineBasicBlock::iterator &MBBI,
+                                     LiveVariables *LV) const;
+

Can you please document the declarations of these functions ?
( see http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments )


+
+/// postRAConvertToLEA - if an instruction can be converted to an
+/// equivalent LEA, insert the new instruction into the basic block
+/// and return a pointer to it. Otherwise, return zero.
+MachineInstr *
+FixupLEAPass::postRAConvertToLEA(MachineFunction::iterator &MFI,
+                                 MachineBasicBlock::iterator &MBBI,
+                                 LiveVariables *LV) const {


Preston,  LiveVariables are not available in this phase and it is only confusing.  Please remove them from the code.

+  LV = getAnalysisIfAvailable<LiveVariables>();  <----- this will always return 0.  Did you ever get a LiveVariable instance ?

+  // Process all basic blocks.
+  for (MachineFunction::iterator I = Func.begin(), E = Func.end(); I != E; ++I)
+    processBasicBlock(Func, I);
+  DEBUG(dbgs() << "End X86FixupLEAs\n";);
+
+  return true;
+}


This function always returns true.  You probably only want to return true of the code changed.

Can you reduce the size of the test cases ?  Ideally I would like to see a single-basic block function. You only need 5-instructions or so. Why did you include such long tests ?



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130425/f1964224/attachment.html>