[llvm-commits] [llvm] r114856 - in /llvm/trunk/lib/Target/ARM: ARMELFWriterInfo.cpp ARMELFWriterInfo.h ARMSubtarget.h ARMTargetMachine.cpp ARMTargetMachine.h CMakeLists.txt

[Jim Grosbach]

On Sep 28, 2010, at 7:10 PM, Rafael Espíndola wrote:

>> That is, just revert the portions of the patch which changed the ARM target and change the X86 target to implement these bits in the same way the ARM target does them.
> 
> That works and a patch implementing it is attached (factor.patch). I
> am not sure if I agree that the "ARM way" is better since it
> introduces more code duplication:
> 
> 98 insertions(+), 77 deletions(-)
> 

Well, a fair bit of those insertions are effectively whitespace. For example, things like:
+  virtual const X86InstrInfo     *getInstrInfo() const {
+    return &InstrInfo;
+  }

vs. 
+  virtual const X86InstrInfo     *getInstrInfo() const { return &InstrInfo; }

Where the latter is preferable in header files (assuming it fits in 80 columns, of course).

Some of the duplication (having those getter methods duplicated at all) is due to the design decision to have the member classes for these things be direct instantiations rather than a pointer which has an object allocated separately in the sub-class and attached to it. It's a trade-off between a bit of duplication for simple methods like this vs. multiple heap allocations, needing to explicitly dealloc them, etc.. Neither is all that pretty, but the current method is probably better?


> A somewhat hackish (can be improved someone likes it) patch that goes
> the other way is also attached (factor2.patch). I like it a bit more
> since it reduces duplication among the targetmachine implementations:

Adding the DataLayout as an explicit member of LLVMTargetMachine is an interesting thought and makes conceptual sense to me. That alone seems like it would solve these ordering issues that you're running into, yes? I like it.

The target data string still needs to be passed down from the subclasses, though, not handled directly by the base target machine class. Likewise the InstrInfo object creation. Moving that stuff to the base class is counter to having the subclasses. Some of the members (that don't need subtarget information beyond what's in the DataLayout, like TSInfo and TLInfo) can still be moved to the base class just fine. The InstrInfo would need to stay in the subclasses since that's where that data is. This would, I think, give us effectively the best of both.

-Jim