[PATCH] D60673: [SimplifyCFG] Improove and speed up ReduceSwitchRange

[James Molloy via Phabricator via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi Shawn,

Thanks for being patient waiting for me to review this. You mention that there are no tests for this behaviour; that's very strange as I remember committing this change with tests . In fact, it was test/Transforms/SimplifyCFG/rangereduce.ll (https://github.com/llvm/llvm-project/commit/b2e436de423aac1de4d764166317dd7ab9cbf751). I'm surprised this test didn't need updating.

Please reupload with full context.

My general comment on this patch is that it's too busy with too few comments to give a good review on. For changes like this I'd really appreciate splitting it into functionally distinct parts. For example here we have:

1. SwitchLookupTable no longer takes an offset and assumes things start from zero and can use an unsigned comparison. There is clearly some change in ReduceSwitchRange that makes this true but I can't unpick the relevant hunk from the rest of this patch. Note, I'm not yet convinced this is correct but the review is too cluttered for me to really understand the logic change.
2. The removal of GCD and use of clz (on APInt) for GCD calculation.
3. The use of clz/cttz
4. Change shift/shift/or to fshr, which I'm certain is going to screw up some backend.

These should really be in separate patches.

Cheers,

James



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5295
     ConstantInt *CaseVal = CI->getCaseValue();
-    if (CaseVal->getValue().slt(MinCaseVal->getValue()))
-      MinCaseVal = CaseVal;
-    if (CaseVal->getValue().sgt(MaxCaseVal->getValue()))
+    if (CaseVal->getValue().ugt(MaxCaseVal->getValue()))
       MaxCaseVal = CaseVal;
----------------
I'm having trouble groking at a glance why this is correct. Could you comment? Previously we had a slt/sgt pair, now just a ugt?

Note that if you'd have uploaded with full context I could go look closer to understand more ;)


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5389
   if (NeedMask) {
-    // Before doing the lookup, we do the hole check. The LookupBB is therefore
-    // re-purposed to do the hole check, and we create a new LookupBB.
-    BasicBlock *MaskBB = LookupBB;
-    MaskBB->setName("switch.hole_check");
-    LookupBB = BasicBlock::Create(Mod.getContext(), "switch.lookup",
-                                  CommonDest->getParent(), CommonDest);
-
-    // Make the mask's bitwidth at least 8-bit and a power-of-2 to avoid
-    // unnecessary illegal types.
-    uint64_t TableSizePowOf2 = NextPowerOf2(std::max(7ULL, TableSize - 1ULL));
-    APInt MaskInt(TableSizePowOf2, 0);
-    APInt One(TableSizePowOf2, 1);
-    // Build bitmask; fill in a 1 bit for every case.
-    const ResultListTy &ResultList = ResultLists[PHIs[0]];
-    for (size_t I = 0, E = ResultList.size(); I != E; ++I) {
-      uint64_t Idx = (ResultList[I].first->getValue() - MinCaseVal->getValue())
-                         .getLimitedValue();
-      MaskInt |= One << Idx;
-    }
-    ConstantInt *TableMask = ConstantInt::get(Mod.getContext(), MaskInt);
-
-    // Get the TableIndex'th bit of the bitmask.
-    // If this bit is 0 (meaning hole) jump to the default destination,
-    // else continue with table lookup.
-    IntegerType *MapTy = TableMask->getType();
-    Value *MaskIndex =
-        Builder.CreateZExtOrTrunc(TableIndex, MapTy, "switch.maskindex");
-    Value *Shifted = Builder.CreateLShr(TableMask, MaskIndex, "switch.shifted");
-    Value *LoBit = Builder.CreateTrunc(
-        Shifted, Type::getInt1Ty(Mod.getContext()), "switch.lobit");
-    Builder.CreateCondBr(LoBit, LookupBB, SI->getDefaultDest());
-
-    Builder.SetInsertPoint(LookupBB);
-    AddPredecessorToBlock(SI->getDefaultDest(), MaskBB, SI->getParent());
+    // Re-written in a later patch
+    return false;
----------------
This can't be committed as-is. If you can't modify the existing code to work in the interim between your two patches, the other patch should be merged into this one.

Note this will make it more of a pain to review, so I'd suggest trying hard to modify the existing code :)


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5466
+  // to make it worth using.
+  const unsigned SubThreshold = (SI->getFunction()->hasOptSize() ? 2 : 8);
+  bool MadeChanges = false;
----------------
I'm having trouble working out how this heuristic relates to the existing code. Have you changed the heuristic as well as the generated code?

Note, a NFC change is much easier to review and get landed.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5482
+  // TODO: this transform would benifit from proper range analysis in SwitchToLookupTable
+  // BitWidth > 8 could be relaxed after this is fixed, but not eliminated, because
+  // this transforms 0 => BitWidth.
----------------
You're describing a tricky gotcha here. Please be more specific and go into more detail as to why it's there in the first place.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5486
+  bool UseCtz = false;
+  if (BitWidth > 8) {
+    unsigned MaxPopCount = 0;
----------------
This whole calculation should be outlined into a helper function. Then for example you wouldn't need to nest the whole thing inside if (BitWidth > 8).


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5495
+      unsigned Ctz = APInt(BitWidth, V).countTrailingZeros();
+      if (Clz < MinClz) MinClz = Clz;
+      if (Clz > MaxClz) MaxClz = Clz;
----------------
use std::min/std::max here?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5500
+    }
+    // Without this check we might do clz followed by ctz, and if Values contains 0,
+    // the result might be woorse.
----------------
Please change the phrasing. Instead of describing what we might do *without* the check, please describe what the goal of the check is (negative vs. positive sense makes it easier to grok what the condition is and whether it matches the descriptive comment).


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5501
+    // Without this check we might do clz followed by ctz, and if Values contains 0,
+    // the result might be woorse.
+    if (MaxPopCount == 1 && Values.back() > 64) {
----------------
> might be worse

...  but why?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5502
+    // the result might be woorse.
+    if (MaxPopCount == 1 && Values.back() > 64) {
+      MadeChanges = true;
----------------
Why > 64?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5504
+      MadeChanges = true;
+      // Prefer clz because it is one instruction cheaper
+      // on ARM, but they cost the same on x86 so if we only need
----------------
This whole comment has me worried. Target-specific guessing games don't belong in SimplifyCFG :)

A heuristic that isn't obviously target-agnostic should get a TTI hook. They're cheap, go for it! :)


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5518
+      // 0 will suddenly become the largest (BitWidth), so we need to sort again.
+      llvm::sort(Values);
+    }
----------------
You're not only mutating but rearranging Values while iterating? this is an antipattern. Please remove.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5584
+      Function *Ctlz = Intrinsic::getDeclaration(SI->getModule(), Intrinsic::ctlz, Ty);
+      ZerosTransform = Builder.Insert(CallInst::Create(Ctlz, {SI->getCondition(), Zero}));
+    } else if (UseCtz) {
----------------
Why not use Builder.CreateCall here?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5594
+    if (Shift) {
+      Function *Fshr = Intrinsic::getDeclaration(SI->getModule(), Intrinsic::fshr, Ty);
+      auto *ShiftC = ConstantInt::get(Ty, Shift);
----------------
How well supported is codegen for fshr? shift/shift/or for rotate will go through pretty much any backend; I've been out of the loop for a little while - how new is fshr?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60673/new/

https://reviews.llvm.org/D60673