[PATCH] Add HLE target feature

[Jeffrey Yasskin]

Nadav, the code he's adding isn't "much more complex"; it's an extra
argument passed through some layers and an extra level of arrays in
some other layers. Your first message said you were judging by the
number of patches, not the complexity of each patch, which is an
incredibly short-sighted way to judge changes: you're explicitly
discouraging contributors from splitting their patches into bite-sized
pieces.

I don't think we can judge whether the cost of HLE primitives as
metadata is worth it unless there's a spec in terms of the LLVM
instruction set, so that's what I asked Michael to provide. It's
possible it will be better to ask him to implement x86 intrinsics
(http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/IntrinsicsX86.td?view=markup)
instead, but we can't know without the IR-level proposal.

On Wed, Feb 27, 2013 at 2:08 PM, Nadav Rotem <nrotem at apple.com> wrote:
> Michael,
>
> I appreciate you rebasing this patch, but this is not helping at all.  I
> asked you specific questions and you ignored them. All of this code that you
> are adding has a cost. Its making the compiler more complex, and it will
> make it difficult for us to refactor major parts of the compiler. Moreover,
> it increases the compile time. Every time that you add special handling for
> some feature you care about you increase the size of the compiler. Every
> feature that you add adds dozens of IFs and hooks that need to be executed
> by innocent people who don't care about this. I asked you a simple question.
> Why does HLE support need to be a part of LLVM ? HLE instructions are
> probably awesome, but they do not belong in the compiler. I could be wrong,
> but it is your responsibility to convince us why we should pay the compile
> time and complexity costs.
>
> Thanks,
> Nadav
>
>
> On Feb 27, 2013, at 1:37 PM, darkbuck <michael.hliao at gmail.com> wrote:
>
>  the patch is rebased to trunk and latest changes on replacing TargetFlags
> to HLEHint
>
> Hi nadav, jfifield, hfinkel, chandlerc, eli.friedman,
>
> http://llvm-reviews.chandlerc.com/D437
>
> CHANGE SINCE LAST DIFF
>  http://llvm-reviews.chandlerc.com/D437?vs=1053&id=1142#toc
>
> Files:
>  lib/Target/X86/X86.td
>  lib/Target/X86/X86InstrInfo.td
>  lib/Target/X86/X86Subtarget.cpp
>  lib/Target/X86/X86Subtarget.h
>
> Index: lib/Target/X86/X86.td
> ===================================================================
> --- lib/Target/X86/X86.td
> +++ lib/Target/X86/X86.td
> @@ -120,6 +120,8 @@
>                                       "Support BMI2 instructions">;
> def FeatureRTM     : SubtargetFeature<"rtm", "HasRTM", "true",
>                                       "Support RTM instructions">;
> +def FeatureHLE     : SubtargetFeature<"hle", "HasHLE", "true",
> +                                      "Support HLE">;
> def FeatureADX     : SubtargetFeature<"adx", "HasADX", "true",
>                                       "Support ADX instructions">;
> def FeatureLeaForSP : SubtargetFeature<"lea-sp", "UseLeaForSP", "true",
> @@ -201,7 +203,7 @@
>                                FeatureRDRAND, FeatureF16C, FeatureFSGSBase,
>                                FeatureMOVBE, FeatureLZCNT, FeatureBMI,
>                                FeatureBMI2, FeatureFMA,
> -                               FeatureRTM]>;
> +                               FeatureRTM, FeatureHLE]>;
>
> def : Proc<"k6",              [FeatureMMX]>;
> def : Proc<"k6-2",            [Feature3DNow]>;
> Index: lib/Target/X86/X86InstrInfo.td
> ===================================================================
> --- lib/Target/X86/X86InstrInfo.td
> +++ lib/Target/X86/X86InstrInfo.td
> @@ -603,6 +603,7 @@
> def HasBMI       : Predicate<"Subtarget->hasBMI()">;
> def HasBMI2      : Predicate<"Subtarget->hasBMI2()">;
> def HasRTM       : Predicate<"Subtarget->hasRTM()">;
> +def HasHLE       : Predicate<"Subtarget->hasHLE()">;
> def HasADX       : Predicate<"Subtarget->hasADX()">;
> def FPStackf32   : Predicate<"!Subtarget->hasSSE1()">;
> def FPStackf64   : Predicate<"!Subtarget->hasSSE2()">;
> Index: lib/Target/X86/X86Subtarget.cpp
> ===================================================================
> --- lib/Target/X86/X86Subtarget.cpp
> +++ lib/Target/X86/X86Subtarget.cpp
> @@ -310,6 +310,10 @@
>         HasBMI = true;
>         ToggleFeature(X86::FeatureBMI);
>       }
> +      if ((EBX >> 4) & 0x1) {
> +        HasHLE = true;
> +        ToggleFeature(X86::FeatureHLE);
> +      }
>       if (IsIntel && ((EBX >> 5) & 0x1)) {
>         X86SSELevel = AVX2;
>         ToggleFeature(X86::FeatureAVX2);
> @@ -439,6 +443,7 @@
>   HasBMI = false;
>   HasBMI2 = false;
>   HasRTM = false;
> +  HasHLE = false;
>   HasADX = false;
>   IsBTMemSlow = false;
>   IsUAMemFast = false;
> Index: lib/Target/X86/X86Subtarget.h
> ===================================================================
> --- lib/Target/X86/X86Subtarget.h
> +++ lib/Target/X86/X86Subtarget.h
> @@ -121,6 +121,9 @@
>   /// HasRTM - Processor has RTM instructions.
>   bool HasRTM;
>
> +  /// HasHLE - Processor has HLE.
> +  bool HasHLE;
> +
>   /// HasADX - Processor has ADX instructions.
>   bool HasADX;
>
> @@ -253,6 +256,7 @@
>   bool hasBMI() const { return HasBMI; }
>   bool hasBMI2() const { return HasBMI2; }
>   bool hasRTM() const { return HasRTM; }
> +  bool hasHLE() const { return HasHLE; }
>   bool hasADX() const { return HasADX; }
>   bool isBTMemSlow() const { return IsBTMemSlow; }
>   bool isUnalignedMemAccessFast() const { return IsUAMemFast; }
> <D437.2.patch>_______________________________________________
>
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>