Modification of bad code in Reassociate.cpp to avoid potential bug.

[alok]

Summary

It is evident in the code itself where variable's (type Instruction) user list is accessed without first checking whether it has at least one element.

Code is rearranged to bring check hasOneUse() before user_back().

[lebedev.ri]

Test?
I'd think Sub->user_back() would simply return nullptr then.

[jmorse]

If I understand correctly, the problem is that calling user_back() might access a container with zero elements, yes? This would appear to be an easy fix -- however IMO you should add reviewers who're familiar with the Reassociate pass better [0]. From a couple of minutes examination it would appear ShouldBreakUpSubtract is only callable from OptimizeInst, and that in turn is guarded by isInstructionTriviallyDead, which I would expect to skip such an instruction. There might be something more complicated broken that someone more familiar with reassociate might spot.

If this really does cause a crash, a regression test would be in order too.

[0] Maybe you have already, I don't recognise all the names, sorry

llvm/lib/Transforms/Scalar/Reassociate.cpp

static bool ShouldBreakUpSubtract(Instruction *Sub) {
   Value *V0 = Sub->getOperand(0);
   if (isReassociableOp(V0, Instruction::Add, Instruction::FAdd) ||
       isReassociableOp(V0, Instruction::Sub, Instruction::FSub))
     return true;
   Value *V1 = Sub->getOperand(1);
   if (isReassociableOp(V1, Instruction::Add, Instruction::FAdd) ||
       isReassociableOp(V1, Instruction::Sub, Instruction::FSub))
     return true;
+  if (Sub->hasOneUse()) {
     Value *VB = Sub->user_back();
-  if (Sub->hasOneUse() &&
-      (isReassociableOp(VB, Instruction::Add, Instruction::FAdd) ||
-       isReassociableOp(VB, Instruction::Sub, Instruction::FSub)))
+    if (isReassociableOp(VB, Instruction::Add, Instruction::FAdd) ||
+        isReassociableOp(VB, Instruction::Sub, Instruction::FSub))
       return true;
+  }
 
   return false;
 }
 
 /// If we have (X-Y), and if either X is an add, or if this is only used by an
 /// add, transform this into (X+(0-Y)) to promote better reassociation.
 static BinaryOperator *BreakUpSubtract(Instruction *Sub,
                                        ReassociatePass::OrderedSet &ToRedo) {