[CFLGraph][NFC] Simplify/reorder switch in visitConstantExpr

[rjmccall]

I'm not sure why I've been CC'ed here, but most of this patch appears to be an arbitrary re-ordering of the cases.

[• dberlin]

I don't understand at all why this is better/simpler

[xbolva00]

In D46259#1084447, @dberlin wrote:

I don't understand at all why this is better/simpler

Same blocks for case labels (see eg. Select and ShuffleVector, but there are more changes) are regrouped together now, the code readability is better I think.

[rjmccall]

Okay, so the actual code changes here are:

• combining the redundant Insert{Element,Value} and Extract{Element,Value} cases, which is good,
• eliminating some unnecessary temporary variables from some of the cases, which is also good, and
• reordering all the cases, which doesn't seem to be an improvement in any way and makes it harder to recognize the first two.

Why don't you submit a patch with just the first two changes?

[xbolva00]

In D46259#1084479, @rjmccall wrote:

Why don't you submit a patch with just the first two changes?

Sorry, updated revision uses the original ordering.

[rjmccall]

Thank you, this makes the change much more obvious. You've still reordered the case starting with BitCast, though.

[xbolva00]

In D46259#1084555, @rjmccall wrote:

Thank you, this makes the change much more obvious. You've still reordered the case starting with BitCast, though.

Missed it, updated to fix it :)

[xbolva00]

Fixed braces

[rjmccall]

I hate to keep making these picky comments, but I don't know what you're trying to do with the braces now. You're adding braces on some of these cases, you're removing them from others, it's all inconsistent. I think it's almost always best to have braces on cases, unless they're just a single-line return or something; so please at least leave the existing braces where they are, and maybe add braces to cases if they don't have them.

[xbolva00]

In D46259#1084581, @rjmccall wrote:

I hate to keep making these picky comments, but I don't know what you're trying to do with the braces now. You're adding braces on some of these cases, you're removing them from others, it's all inconsistent. I think it's almost always best to have braces on cases, unless they're just a single-line return or something; so please at least leave the existing braces where they are, and maybe add braces to cases if they don't have them.

It was inconsistent even before :/

Current code has the pattern like "If two addX in the case then use braces, if just one, no braces" so I was not sure. I added them at first and then I removed them just becauce I thought you would blame me that code doesnt follow original "scheme/pattern".

Sorry, thanks for the advice, I will fix it.

[rjmccall]

Thanks, this looks great.

[efriedma]

llvm/trunk/lib/Analysis/CFLGraph.h

548 ↗

(On Diff #144792)

The logic for selects looks suspicious; operand 0 is the condition.

[xbolva00]

llvm/trunk/lib/Analysis/CFLGraph.h

548 ↗

(On Diff #144792)

Yep true. Seems like this is quite an old bug here.

We should use operands 1 and 2 here.

I will post a new patch tomorrow.

[xbolva00]

llvm/trunk/lib/Analysis/CFLGraph.h

548 ↗

(On Diff #144792)

Can you look on it?

https://reviews.llvm.org/D46343

Thanks :)

lib/Analysis/CFLGraph.h

void visitShuffleVectorInst(ShuffleVectorInst &Inst) {
       auto *From1 = Inst.getOperand(0);
       auto *From2 = Inst.getOperand(1);
       addAssignEdge(From1, &Inst);
       addAssignEdge(From2, &Inst);
     }
 
     void visitConstantExpr(ConstantExpr *CE) {
       switch (CE->getOpcode()) {
-      case Instruction::GetElementPtr: {
-        auto GEPOp = cast<GEPOperator>(CE);
-        visitGEP(*GEPOp);
-        break;
-      }
-      case Instruction::PtrToInt: {
-        auto *Ptr = CE->getOperand(0);
-        addNode(Ptr, getAttrEscaped());
-        break;
-      }
-      case Instruction::IntToPtr:
-        addNode(CE, getAttrUnknown());
+
+      case Instruction::Add:
+      case Instruction::Sub:
+      case Instruction::FSub:
+      case Instruction::Mul:
+      case Instruction::FMul:
+      case Instruction::UDiv:
+      case Instruction::SDiv:
+      case Instruction::FDiv:
+      case Instruction::URem:
+      case Instruction::SRem:
+      case Instruction::FRem:
+      case Instruction::And:
+      case Instruction::Or:
+      case Instruction::Xor:
+      case Instruction::Shl:
+      case Instruction::LShr:
+      case Instruction::AShr:
+      case Instruction::ICmp:
+      case Instruction::FCmp:
+      case Instruction::Select:
+      case Instruction::ShuffleVector:
+        addAssignEdge(CE->getOperand(0), CE);
+        addAssignEdge(CE->getOperand(1), CE);
         break;
 
       case Instruction::BitCast:
       case Instruction::AddrSpaceCast:
       case Instruction::Trunc:
       case Instruction::ZExt:
       case Instruction::SExt:
       case Instruction::FPExt:
       case Instruction::FPTrunc:
       case Instruction::UIToFP:
       case Instruction::SIToFP:
       case Instruction::FPToUI:
       case Instruction::FPToSI: {
-        auto *Src = CE->getOperand(0);
-        addAssignEdge(Src, CE);
+        addAssignEdge(CE->getOperand(0), CE);
         break;
       }
-      case Instruction::Select: {
-        auto *TrueVal = CE->getOperand(0);
-        auto *FalseVal = CE->getOperand(1);
-        addAssignEdge(TrueVal, CE);
-        addAssignEdge(FalseVal, CE);
+
+      case Instruction::GetElementPtr: {
+        auto GEPOp = cast<GEPOperator>(CE);
+        visitGEP(*GEPOp);
         break;
       }
-      case Instruction::InsertElement: {
-        auto *Vec = CE->getOperand(0);
-        auto *Val = CE->getOperand(1);
-        addAssignEdge(Vec, CE);
-        addStoreEdge(Val, CE);
+
+      case Instruction::PtrToInt: {
+        addNode(CE->getOperand(0), getAttrEscaped());
         break;
       }
-      case Instruction::ExtractElement: {
-        auto *Ptr = CE->getOperand(0);
-        addLoadEdge(Ptr, CE);
+      case Instruction::IntToPtr:
+        addNode(CE, getAttrUnknown());
         break;
-      }
+
+      case Instruction::InsertElement:
       case Instruction::InsertValue: {
-        auto *Agg = CE->getOperand(0);
-        auto *Val = CE->getOperand(1);
-        addAssignEdge(Agg, CE);
-        addStoreEdge(Val, CE);
+        addAssignEdge(CE->getOperand(0), CE);
+        addStoreEdge(CE->getOperand(1), CE);
         break;
       }
+
+      case Instruction::ExtractElement:
       case Instruction::ExtractValue: {
-        auto *Ptr = CE->getOperand(0);
-        addLoadEdge(Ptr, CE);
+        addLoadEdge(CE->getOperand(0), CE);
         break;
       }
-      case Instruction::ShuffleVector: {
-        auto *From1 = CE->getOperand(0);
-        auto *From2 = CE->getOperand(1);
-        addAssignEdge(From1, CE);
-        addAssignEdge(From2, CE);
-        break;
-      }
-      case Instruction::Add:
-      case Instruction::Sub:
-      case Instruction::FSub:
-      case Instruction::Mul:
-      case Instruction::FMul:
-      case Instruction::UDiv:
-      case Instruction::SDiv:
-      case Instruction::FDiv:
-      case Instruction::URem:
-      case Instruction::SRem:
-      case Instruction::FRem:
-      case Instruction::And:
-      case Instruction::Or:
-      case Instruction::Xor:
-      case Instruction::Shl:
-      case Instruction::LShr:
-      case Instruction::AShr:
-      case Instruction::ICmp:
-      case Instruction::FCmp:
-        addAssignEdge(CE->getOperand(0), CE);
-        addAssignEdge(CE->getOperand(1), CE);
-        break;
 
       default:
         llvm_unreachable("Unknown instruction type encountered!");
       }
     }
   };
 
   // Helper functions