[llvm-commits] [llvm] r109359 - in /llvm/trunk: include/llvm/Target/TargetLowering.h lib/Target/ARM/ARM.h lib/Target/ARM/ARMGlobalMerge.cpp lib/Target/ARM/ARMISelLowering.cpp lib/Target/ARM/ARMISelLowering.h lib/Target/ARM/ARMTargetMachine.cpp lib/Target/ARM/ARMTargetMachine.h

[Chris Lattner]

On Jul 24, 2010, at 2:52 PM, Anton Korobeynikov wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=109359&view=rev
> Log:
> Hook in GlobalMerge pass

Hey Anton,

Sorry for the delay reviewing this.  My general thought on this is that it should be a target-independent pass.  PowerPC would certainly benefit from this as well, probably x86 in PIC mode as well, among others.  Can this be generalized to support other targets?

Also, you added no regression tests for this.  Please fix this, this is a major new feature, and we want to make sure it is not broken in the future.

> +++ llvm/trunk/include/llvm/Target/TargetLowering.h Sat Jul 24 16:52:08 2010
> @@ -792,6 +792,12 @@
>     return false;
>   }
> 
> +  /// getMaximalGlobalOffset - Returns the maximal possible offset which can be
> +  /// used for loads / stores from the global.
> +  virtual unsigned getMaximalGlobalOffset() const {
> +    return 0;
> +  }

If this stays an ARM-specific pass, there is no reason to have this as a target hook.  If the hook stays, please improve the comment to better describe what this means and what is says.  

Could you just use the existing hooks that LSR uses to determine reg+imm offsets?  The "offset from a global" depends on the type being accessed and many other things, so it isn't clear how to define this hook.  Using the existing LSR hooks seems beneficial because if you end up turning [reg1+reg2] into tmp = reg1+imm [tmp+reg2] addressing, you actually don't save a register.  I understand that you're not modeling this yet, but in any case, fewer hooks is better.

-Chris