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

[Chris Lattner]

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