[llvm-commits] [llvm] r100910 - in /llvm/trunk/tools: Makefile edis/EDDisassembler.cpp edis/Makefile

[Sean Callanan]

Chris,

I agree that this is a hack, and I will fix it.  Right now I'm trying to figure out how to make the tester work.  Then I will make a pass through everything and convert the hard-#including into calls into the MCDisassembler returning the appropriate data.

Sean

On Apr 11, 2010, at 7:15 PM, Chris Lattner wrote:

> 
> On Apr 9, 2010, at 5:48 PM, Sean Callanan wrote:
> 
>> Author: spyffe
>> Date: Fri Apr  9 19:48:10 2010
>> New Revision: 100910
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=100910&view=rev
>> Log:
>> Updated the edis build mechanism to allow for builds
>> that do not build some (or all) of the targets that
>> edis supports.
> 
> Hi Sean,
> 
>> +++ llvm/trunk/tools/Makefile Fri Apr  9 19:48:10 2010
>> @@ -36,7 +36,7 @@
>> ifeq ($(ENABLE_PIC),1)
>>  # No support for dynamic libraries on windows targets.
>>  ifneq ($(TARGET_OS), $(filter $(TARGET_OS), Cygwin MingW))
>> -    PARALLEL_DIRS += edis
>> +    DIRS += edis
>> 
> 
> Why this change?  If it is right, it definitely deserves a comment about why it can't be done in parallel.
> 
>> +++ llvm/trunk/tools/edis/EDDisassembler.cpp Fri Apr  9 19:48:10 2010
>> @@ -39,8 +39,13 @@
>> #include "llvm/Target/TargetRegisterInfo.h"
>> #include "llvm/Target/TargetSelect.h"
>> 
>> +#ifdef EDIS_X86
>> #include "../../lib/Target/X86/X86GenEDInfo.inc"
>> +#endif
>> +
>> +#ifdef EDIS_ARM
>> #include "../../lib/Target/ARM/ARMGenEDInfo.inc"
>> +#endif
> 
> As Daniel said, this is a layering violation.  Please add the right APIs to TargetRegistry.h.
> 
>> 
>> using namespace llvm;
>> 
>> @@ -54,10 +59,14 @@
>> };
>> 
>> static struct InfoMap infomap[] = {
>> +#ifdef EDIS_X86
>>  { Triple::x86,          "i386-unknown-unknown",   instInfoX86 },
>>  { Triple::x86_64,       "x86_64-unknown-unknown", instInfoX86 },
>> +#endif
>> +#ifdef EDIS_ARM
>>  { Triple::arm,          "arm-unknown-unknown",    instInfoARM },
>>  { Triple::thumb,        "thumb-unknown-unknown",  instInfoARM },
>> +#endif
> 
> This is a hack, all this should be in the targets :).  Adding a new target shouldn't require changing edis code itself and we have good infrastructure for dealing with this sort of thing in TargetRegistry.
> 
>> 
>> +++ llvm/trunk/tools/edis/Makefile Fri Apr  9 19:48:10 2010
>> @@ -45,6 +45,19 @@
>>    endif
>> endif
>> 
>> +EDIS_DEFINES = 
>> +
>> +ifneq (,$(findstring X86,$(TARGETS_TO_BUILD)))
>> +	EDIS_DEFINES := $(EDIS_DEFINES) -DEDIS_X86
>> +endif
>> +
>> +ifneq (,$(findstring ARM,$(TARGETS_TO_BUILD)))
>> +	EDIS_DEFINES := $(EDIS_DEFINES) -DEDIS_ARM
>> +endif
>> +
>> +CXXFLAGS := $(CXXFLAGS) 
>> +#$(EDIS_DEFINES)
>> +
> 
> Doing this right will also eliminate the need for this stuff.
> 
> -Chris
>