[PATCH] D41029: [JumpTables][PowerPC] Let targets decide which switch instructions are suitable for jump tables

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1162
+          case Intrinsic::ppc_mtctr:
+            BlocksKnownToUseCTR.insert(BB);
+            return true;
----------------
I feel like any time this would be true in a block terminated by a switch is a bug, and not using a jump table at all just because some case happens to jump to a loop pre-header seems very severe. Maybe we just want to raise the number of entries in the table in that case?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.h:539
+    mutable std::set<const BasicBlock*> BlocksKnownToUseCTR;
+    mutable std::set<const BasicBlock*> BlocksKnownNotToUseCTR;
 
----------------
efriedma wrote:
> We generally prefer SmallPtrSet for sets of pointers.  And for long-lived pointers to LLVM values, it's generally a good idea to use AssertingVH (so something like `SmallPtrSet<AssertingVH<SwitchInst>>`).
> 
> We share the same PPCTargetLowering for multiple functions, so you'll build up a set for the entire module in most cases.  But I guess that isn't really a problem.
Please use a SmallPtrSet here. In LLVM, we only use std::set if we need some property it offers (e.g, ordered iteration or stable addressing). Otherwise, we don't. That actually makes the semantics clearer, but only if we're consistent.


Repository:
  rL LLVM

https://reviews.llvm.org/D41029