llvm dev - Dec 2014 - [LLVMdev] [RFC] Stripping unusable intrinsics

On Dec 23, 2014, at 10:28 AM, Chris Bieneman <beanz at apple.com>
wrote:
>>> It should be straight-forward to have something like
LLVMInitializeX86Target/RegisterTargetMachine install the intrinsics into a
registry.
>> 
>> I tried doing that a few years ago. It’s not nearly as easy as it
sounds because we’ve got hardcoded references to various target intrinsics
scattered throughout the code.
> 
> I was just writing to say exactly this. There are a number of ways we could
work toward something like this. I’m completely in favor of a world where
Intrinsics are properties of the targets and don’t leach out, however today they
do in a lot of places.
What are the specific problems here?  Anything that does an equality comparison
with the IntrinsicID can be changed to do strcmp with the name.  That would
handle the one-off cases like InstCombiner::SimplifyDemandedUseBits in
InstCombine.

The other cases in InstCombine could be handled similarly, but may be better
handled by adding a intrinsic behavior query APIs to the intrinsic registry, or
would be better handled (eventually) by adding new attributes to the intrinsics
themselves.

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20141223/d738d619/attachment.html>

> On Dec 23, 2014, at 10:41 AM, Chris Lattner <clattner at apple.com>
wrote:
> 
> On Dec 23, 2014, at 10:28 AM, Chris Bieneman <beanz at apple.com
<mailto:beanz at apple.com>> wrote:
>>>> It should be straight-forward to have something like
LLVMInitializeX86Target/RegisterTargetMachine install the intrinsics into a
registry.
>>> 
>>> I tried doing that a few years ago. It’s not nearly as easy as it
sounds because we’ve got hardcoded references to various target intrinsics
scattered throughout the code.
>> 
>> I was just writing to say exactly this. There are a number of ways we
could work toward something like this. I’m completely in favor of a world where
Intrinsics are properties of the targets and don’t leach out, however today they
do in a lot of places.
> 
> What are the specific problems here?  Anything that does an equality
comparison with the IntrinsicID can be changed to do strcmp with the name.  That
would handle the one-off cases like InstCombiner::SimplifyDemandedUseBits in
InstCombine.
> 
> The other cases in InstCombine could be handled similarly, but may be
better handled by adding a intrinsic behavior query APIs to the intrinsic
registry, or would be better handled (eventually) by adding new attributes to
the intrinsics themselves.
I don’t think there’s anything fundamentally difficult, but it’s a big change.
For example:

$ git grep Intrinsic:: | wc
    3364   12286  281078

The vast majority of those 3,364 lines have hardcoded references to specific
intrinsics. Many of them are used in contexts where you can’t easily insert a
strcmp (e.g., case values in large switch statements, or worse, the m_Intrinsic
PatternMatch templates).
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20141223/5a62999b/attachment.html>

I had patches earlier on this thread that stripped the uses of intrinsics from
LLVM using ifdefs.

These files have intrinsics as switch cases. I’ve listed which target the
intrinsics belong to as well for reference.

lib/Analysis/BasicAliasAnalysis.cpp - ARM
lib/Analysis/ConstantFolding.cpp - X86
lib/Analysis/ValueTracking.cpp - X86
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp - X86
lib/Transforms/InstCombine/InstCombineCalls.cpp - PowerPC, X86, ARM, AArch64,
and R600
lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp - X86
lib/Transforms/Instrumentation/MemorySanitizer.cpp - X86
lib/Transforms/Scalar/LoopStrengthReduce.cpp - X86

In InstCombineCalls some of the case bodies are shared by ARM and AArch64
intrinsics. In those cases there were also some if statements that matched on
the intrinsic enums too. As an example see the case at InstCombineCalls.cpp line
922-1028.

Other than that, lib/IR/AutoUpgrade.cpp has a bunch of string matched X86
Intrinsics being mapped to enums.

Clang also has a bunch of places that use target intrinsics that were not as
easy to ifdef, so I didn’t try.

As Bob said, I don’t think this is particularly a difficult undertaking, but it
will be a big change. I think a lot of the switch statements could be replaced
by per-target hooks.

Personally I’d love to be in a world where intrinsics lived in the targets, but
we won’t get there quickly. I think a stop-gap solution for our more immediate
needs could be accomplished with Alex’s suggestion of a clang attribute. I’ve
sent patches to clang (http://reviews.llvm.org/D6763
<http://reviews.llvm.org/D6763>). One of the allures I see to that
approach is that it would allow us to get what we need today without significant
modifications to LLVM, and it would not get in the way of working towards the
“Right™” solution.

-Chris
> On Dec 23, 2014, at 10:54 AM, Bob Wilson <bob.wilson at apple.com>
wrote:
> 
>> 
>> On Dec 23, 2014, at 10:41 AM, Chris Lattner <clattner at apple.com
<mailto:clattner at apple.com>> wrote:
>> 
>> On Dec 23, 2014, at 10:28 AM, Chris Bieneman <beanz at apple.com
<mailto:beanz at apple.com>> wrote:
>>>>> It should be straight-forward to have something like
LLVMInitializeX86Target/RegisterTargetMachine install the intrinsics into a
registry.
>>>> 
>>>> I tried doing that a few years ago. It’s not nearly as easy as
it sounds because we’ve got hardcoded references to various target intrinsics
scattered throughout the code.
>>> 
>>> I was just writing to say exactly this. There are a number of ways
we could work toward something like this. I’m completely in favor of a world
where Intrinsics are properties of the targets and don’t leach out, however
today they do in a lot of places.
>> 
>> What are the specific problems here?  Anything that does an equality
comparison with the IntrinsicID can be changed to do strcmp with the name.  That
would handle the one-off cases like InstCombiner::SimplifyDemandedUseBits in
InstCombine.
>> 
>> The other cases in InstCombine could be handled similarly, but may be
better handled by adding a intrinsic behavior query APIs to the intrinsic
registry, or would be better handled (eventually) by adding new attributes to
the intrinsics themselves.
> 
> I don’t think there’s anything fundamentally difficult, but it’s a big
change. For example:
> 
> $ git grep Intrinsic:: | wc
>     3364   12286  281078
> 
> The vast majority of those 3,364 lines have hardcoded references to
specific intrinsics. Many of them are used in contexts where you can’t easily
insert a strcmp (e.g., case values in large switch statements, or worse, the
m_Intrinsic PatternMatch templates).
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20141223/125226f3/attachment.html>

> On Dec 23, 2014, at 10:54 AM, Bob Wilson <bob.wilson at apple.com>
wrote:
> 
> 
>> On Dec 23, 2014, at 10:41 AM, Chris Lattner <clattner at apple.com
<mailto:clattner at apple.com>> wrote:
>> 
>> On Dec 23, 2014, at 10:28 AM, Chris Bieneman <beanz at apple.com
<mailto:beanz at apple.com>> wrote:
>>>>> It should be straight-forward to have something like
LLVMInitializeX86Target/RegisterTargetMachine install the intrinsics into a
registry.
>>>> 
>>>> I tried doing that a few years ago. It’s not nearly as easy as
it sounds because we’ve got hardcoded references to various target intrinsics
scattered throughout the code.
>>> 
>>> I was just writing to say exactly this. There are a number of ways
we could work toward something like this. I’m completely in favor of a world
where Intrinsics are properties of the targets and don’t leach out, however
today they do in a lot of places.
>> 
>> What are the specific problems here?  Anything that does an equality
comparison with the IntrinsicID can be changed to do strcmp with the name.  That
would handle the one-off cases like InstCombiner::SimplifyDemandedUseBits in
InstCombine.
>> 
>> The other cases in InstCombine could be handled similarly, but may be
better handled by adding a intrinsic behavior query APIs to the intrinsic
registry, or would be better handled (eventually) by adding new attributes to
the intrinsics themselves.
> 
> I don’t think there’s anything fundamentally difficult, but it’s a big
change. For example:
> 
> $ git grep Intrinsic:: | wc
>     3364   12286  281078
> 
> The vast majority of those 3,364 lines have hardcoded references to
specific intrinsics. Many of them are used in contexts where you can’t easily
insert a strcmp (e.g., case values in large switch statements, or worse, the
m_Intrinsic PatternMatch templates).
I don’t find this convincing.  It should be simple to introduce a new
m_Intrinsic PatternMatch template that takes a string.  The switches are also
straight-forward.  In BasicAA, for example, instead of:

   switch (II->getIntrinsicID()) {
    default: break;
    case Intrinsic::memset:
    case Intrinsic::memcpy:
    case Intrinsic::memmove: {
    ...
    case Intrinsic::arm_neon_vld1: {
      assert(ArgIdx == 0 && "Invalid argument index");
      assert(Loc.Ptr == II->getArgOperand(ArgIdx) &&
             "Intrinsic location pointer not argument?");
      // LLVM's vld1 and vst1 intrinsics currently only support a single
      // vector register.
      if (DL)
        Loc.Size = DL->getTypeStoreSize(II->getType());
      break;
    }
}

Just change this to:

   switch (II->getIntrinsicID()) {
    case Intrinsic::memset:
    case Intrinsic::memcpy:
    case Intrinsic::memmove: {
    ...
    default:
      if (II->getName() == “llvm.arm.neon.vld1”) {
        same stuff as above
      }
    break;

Spreading ifdefs through the codebase would be really unfortunate.

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20141223/eb3be06b/attachment.html>

On 12/23/2014 10:41 AM, Chris Lattner wrote:
> On Dec 23, 2014, at 10:28 AM, Chris Bieneman <beanz at apple.com 
> <mailto:beanz at apple.com>> wrote:
>>>> It should be straight-forward to have something like 
>>>> LLVMInitializeX86Target/RegisterTargetMachine install the 
>>>> intrinsics into a registry.
>>>
>>> I tried doing that a few years ago. It’s not nearly as easy as it 
>>> sounds because we’ve got hardcoded references to various target 
>>> intrinsics scattered throughout the code.
>>
>> I was just writing to say exactly this. There are a number of ways we 
>> could work toward something like this. I’m completely in favor of a 
>> world where Intrinsics are properties of the targets and don’t leach 
>> out, however today they do in a lot of places.
>
> What are the specific problems here?  Anything that does an equality 
> comparison with the IntrinsicID can be changed to do strcmp with the 
> name.  That would handle the one-off cases like 
> InstCombiner::SimplifyDemandedUseBits in InstCombine.
>
> The other cases in InstCombine could be handled similarly, but may be 
> better handled by adding a intrinsic behavior query APIs to the 
> intrinsic registry, or would be better handled (eventually) by adding 
> new attributes to the intrinsics themselves.
I find the idea of replacing a bunch of case statements in a switch with 
string comparisons to be highly objectionable.  This would be a marked 
decrease both in readability and maintainability.  I also don't see how 
it helps reduce code bloat.

Let me make an alternate proposal.  Let's separate out the target 
specific cases into their own switch.  We can introduce a function along 
the lines of isTargetArchitectureEnabled(X86) which internally is 
implemented via macros, but externally is just a function call.

We then have code that looks like:
switch(intrinsic_id) {
case Intrinsic::llvm_X: {}
case ....
};
if (isTargetArchitectureEnabled(X86)) {
   // We could even break this into a helper function...
   switch(intrinsic_id) {
   case Intrinsic::x86_X: {}
   case ....
   };
}

If we've done our jobs right in the optimizer, we should be able to 
constant fold that condition at compile time and merge the active 
switches back into a single switch case.  If the architecture isn't 
enabled, the code will trivially fold away.  All without (visible) 
macros or changing the use of intrinsic ids.

Philip






-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20141229/601cd091/attachment.html>