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>