[PATCH] D101103: [InstSimplify] Treat invariant group insts as bitcasts for load operands

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

I think the recursion is unfortunate there. How about something like

  static Constant *ConstructLoadOperandConstant(Value *Op) {
    SmallVector<Value*, 16> Worklist;
    
    OpStack.emplace_back(Op);
    for(int I = 0; I != OpStack.size(); ++I) {
      Value* Op = OpStack.back();
      
      if (isa<Constant>(Op))
        break; // YAY!
  
      if (auto *BC = dyn_cast<BitCastOperator>(Op))
        OpStack.emplace_back(BC->getOperand(0)); // Recurse.
  
      if (auto *GEP = dyn_cast<GEPOperator>(Op)) {
        if (all_of(ArrayRef<Value*>(GEP->operands()).drop_front(),
            [](Value*Op) { return isa<Constant>(Op); }))
          OpStack.emplace_back(GEP->getOperand(0)); // Recurse.
      }
  
      if (auto *II = dyn_cast<IntrinsicInst>(Op)) {
        if (II->getIntrinsicID() == Intrinsic::strip_invariant_group ||
            II->getIntrinsicID() == Intrinsic::launder_invariant_group)
          OpStack.emplace_back(II->getOperand(0)); // Recurse.
      }
      
      return nullptr; // Can't look past this instruction. Give up.
    }
    
    // Alright! Reconstruct this chain as constant expressions.
    Constant* NewOp = cast<Constant>(Worklist.pop_back_val());
    while(!OpStack.empty()) {
      Value* Op = OpStack.pop_back_val();
  
      if (isa<BitCastOperator>(Op))
        NewOp = ConstantExpr::getBitCast(NewOp, Op->getType());
  
      if (auto *GEP = dyn_cast<GEPOperator>(Op)) {
        SmallVector<Constant *> Idxs;
        Idxs.reserve(GEP->getNumOperands() - 1);
        for (unsigned I = 1, E = GEP->getNumOperands(); I != E; ++I)
          Idxs.push_back(cast<Constant>(GEP->getOperand(I));
        return ConstantExpr::getGetElementPtr(GEP->getSourceElementType(), NewOp,
                                              Idxs, GEP->isInBounds(),
                                              GEP->getInRangeIndex());
      }
  
      if (auto *II = dyn_cast<IntrinsicInst>(Op)) {
        if (II->getIntrinsicID() == Intrinsic::strip_invariant_group ||
            II->getIntrinsicID() == Intrinsic::launder_invariant_group)
          NewOp = ConstantExpr::getBitCast(NewOp, Op->getType());  
      }
      
      llvm_unreachable("Instruction handled in first loop but not here?");
    }
  }
    
  return NewOp;



================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:5855
 
+static Constant *ConstructLoadOperandConstant(Value *Op) {
+  if (auto *C = dyn_cast<Constant>(Op))
----------------
Given load from \p Op, see if we can peel off certain pointer adjustments,
that are no-op for the purpose of the performing the load,
in the hope of eventually discovering that we are
actually loading from a constant.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:5893-5894
+static Value *SimplifyLoadInst(LoadInst *LI, const SimplifyQuery &Q) {
+  if (auto *C = dyn_cast<Constant>(LI->getPointerOperand()))
+    return ConstantFoldLoadFromConstPtr(C, LI->getType(), Q.DL);
+
----------------
As per D33619. and the default of the switch in `SimplifyInstruction()`,
we'll no longer call `ConstantFoldInstruction()`, but just the `ConstantFoldLoadFromConstPtr()`.
Will we loose anything important because of that?

I would guess that iff we actually want to change that, perhaps it should be done afterwards,
and this should just continue to use `ConstantFoldInstruction()`?


================
Comment at: llvm/test/Transforms/InstSimplify/invariant.group-load.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -passes=instsimplify -S < %s | FileCheck %s
+
----------------
I'd like to see a bit more tests.
For example, this really needs negative tests.
What should happen if the `@A` isn't a `constant`?
One test with some pattern we can't handle would be good (non-constant gep indexes e.g.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101103