Handle ConstantExpr correctly in SelectionDAGBuilder

[Praetonus]

Summary

This change fixes a bug in SelectionDAGBuilder::visitInsertValue and SelectionDAGBuilder::visitExtractValue where constant expressions (InsertValueConstantExpr and ExtractValueConstantExpr) would be treated as non-constant instructions (InsertValueInst and ExtractValueInst). This bug resulted in an incorrect memory access, which manifested as an assertion failure in SDValue::SDValue.

Fixes PR#33094.

[efriedma]

Missing testcase... although, I'm not how you would write one.

Could we fix SelectionDAGBuilder::visit so this problem can't happen again?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

3228 ↗

(On Diff #103643)

"else", instead of "else if"?

[davide]

I didn't read this fix so I can't say for sure but there's a reduced test in the original PR
https://bugs.llvm.org/show_bug.cgi?id=33094#c2
so if that doesn't crash anymore, it should be better than no test at all.

[Praetonus]

@efriedma Sorry for the test case, I did write one based on the reduced case from the PR, I'm not sure why SVN didn't include it in the diff. Anyway, I'll update to include it.

I'm not sure how SelectionDAGBuilder::visit could be fixed. It seems to me that instruction opcodes are shared by both the constant and non-constant version of an instruction, so the function wouldn't be able to handle a constant and a non-constant instruction separately unless it uses isa or a similar mechanism. I could be missing something though since my knowledge of the LLVM code base is rather limited.

[Praetonus]

A test case has been added and the patch has been updated based on @efriedma's comments.

[efriedma]

I'll think about the SelectionDAGBuilder::visit issue myself, and maybe come up with a followup patch.

test/CodeGen/Generic/pr33094.ll

2 ↗

(On Diff #103662)

Needs a comment describing what it's testing.

Writing tests without any CHECK lines is usually a bad idea, because they tend to be very easy to break, but I'm not sure what we would want to check for here, exactly, so I guess this is okay.

[RKSimon]

test/CodeGen/Generic/pr33094.ll

2 ↗

(On Diff #103662)

Shouldn't this at least include a REQUIRES: asserts?

[efriedma]

test/CodeGen/Generic/pr33094.ll

2 ↗

(On Diff #103662)

Why would we want "REQUIRES: asserts"? The test isn't checking debug output, or anything like that.

[Praetonus]

I've added a comment describing the test case.

[efriedma]

LGTM.

[Praetonus]

Sorry, I forgot to mention that I don't have commit access. Could somebody commit this on my behalf?

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

 class AddrSpaceCastInst;
 class AllocaInst;
 class BasicBlock;
 class BitCastInst;
 class BranchInst;
 class CallInst;
 class DbgValueInst;
 class ExtractElementInst;
-class ExtractValueInst;
 class FCmpInst;
 class FPExtInst;
 class FPToSIInst;
 class FPToUIInst;
 class FPTruncInst;
 class Function;
 class FunctionLoweringInfo;
 class GetElementPtrInst;
 class GCFunctionInfo;
 class ICmpInst;
 class IntToPtrInst;
 class IndirectBrInst;
 class InvokeInst;
 class InsertElementInst;
-class InsertValueInst;
 class Instruction;
 class LoadInst;
 class MachineBasicBlock;
 class MachineInstr;
 class MachineRegisterInfo;
 class MDNode;
 class MVT;
 class PHINode;
private:
   void visitIntToPtr(const User &I);
   void visitBitCast(const User &I);
   void visitAddrSpaceCast(const User &I);
 
   void visitExtractElement(const User &I);
   void visitInsertElement(const User &I);
   void visitShuffleVector(const User &I);
 
-  void visitExtractValue(const ExtractValueInst &I);
-  void visitInsertValue(const InsertValueInst &I);
+  void visitExtractValue(const User &I);
+  void visitInsertValue(const User &I);
   void visitLandingPad(const LandingPadInst &I);
 
   void visitGetElementPtr(const User &I);
   void visitSelect(const User &I);
 
   void visitAlloca(const AllocaInst &I);
   void visitLoad(const LoadInst &I);
   void visitStore(const StoreInst &I);

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

for (int Idx : Mask) {
     }
 
     Ops.push_back(Res);
   }
 
   setValue(&I, DAG.getBuildVector(VT, DL, Ops));
 }
 
-void SelectionDAGBuilder::visitInsertValue(const InsertValueInst &I) {
+void SelectionDAGBuilder::visitInsertValue(const User &I) {
+  ArrayRef<unsigned> Indices;
+  if (const InsertValueInst *IV = dyn_cast<InsertValueInst>(&I))
+    Indices = IV->getIndices();
+  else
+    Indices = cast<ConstantExpr>(&I)->getIndices();
+
   const Value *Op0 = I.getOperand(0);
   const Value *Op1 = I.getOperand(1);
   Type *AggTy = I.getType();
   Type *ValTy = Op1->getType();
   bool IntoUndef = isa<UndefValue>(Op0);
   bool FromUndef = isa<UndefValue>(Op1);
 
-  unsigned LinearIndex = ComputeLinearIndex(AggTy, I.getIndices());
+  unsigned LinearIndex = ComputeLinearIndex(AggTy, Indices);
 
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   SmallVector<EVT, 4> AggValueVTs;
   ComputeValueVTs(TLI, DAG.getDataLayout(), AggTy, AggValueVTs);
   SmallVector<EVT, 4> ValValueVTs;
   ComputeValueVTs(TLI, DAG.getDataLayout(), ValTy, ValValueVTs);
 
   unsigned NumAggValues = AggValueVTs.size();
void SelectionDAGBuilder::visitInsertValue(const User &I) {
   for (; i != NumAggValues; ++i)
     Values[i] = IntoUndef ? DAG.getUNDEF(AggValueVTs[i]) :
                 SDValue(Agg.getNode(), Agg.getResNo() + i);
 
   setValue(&I, DAG.getNode(ISD::MERGE_VALUES, getCurSDLoc(),
                            DAG.getVTList(AggValueVTs), Values));
 }
 
-void SelectionDAGBuilder::visitExtractValue(const ExtractValueInst &I) {
+void SelectionDAGBuilder::visitExtractValue(const User &I) {
+  ArrayRef<unsigned> Indices;
+  if (const ExtractValueInst *EV = dyn_cast<ExtractValueInst>(&I))
+    Indices = EV->getIndices();
+  else
+    Indices = cast<ConstantExpr>(&I)->getIndices();
+
   const Value *Op0 = I.getOperand(0);
   Type *AggTy = Op0->getType();
   Type *ValTy = I.getType();
   bool OutOfUndef = isa<UndefValue>(Op0);
 
-  unsigned LinearIndex = ComputeLinearIndex(AggTy, I.getIndices());
+  unsigned LinearIndex = ComputeLinearIndex(AggTy, Indices);
 
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   SmallVector<EVT, 4> ValValueVTs;
   ComputeValueVTs(TLI, DAG.getDataLayout(), ValTy, ValValueVTs);
 
   unsigned NumValValues = ValValueVTs.size();
 
   // Ignore a extractvalue that produces an empty object

llvm/trunk/test/CodeGen/Generic/pr33094.ll

+; RUN: llc < %s
+
+; PR33094
+; Make sure that a constant extractvalue doesn't cause a crash in
+; SelectionDAGBuilder::visitExtractValue.
+
+%A = type {}
+%B = type {}
+%Tuple = type { i64 }
+
+@A_Inst = global %A zeroinitializer
+@B_Inst = global %B zeroinitializer
+
+define i64 @foo() {
+  ret i64 extractvalue (%Tuple select (i1 icmp eq
+                        (%B* bitcast (%A* @A_Inst to %B*), %B* @B_Inst),
+                        %Tuple { i64 33 }, %Tuple { i64 42 }), 0)
+}