[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

public:
 
     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());
+        addNode(CE->getOperand(0), getAttrEscaped());
         break;
       }
-      case Instruction::IntToPtr:
+
+      case Instruction::IntToPtr: {
         addNode(CE, getAttrUnknown());
         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);
-        break;
-      }
-      case Instruction::Select: {
-        auto *TrueVal = CE->getOperand(0);
-        auto *FalseVal = CE->getOperand(1);
-        addAssignEdge(TrueVal, CE);
-        addAssignEdge(FalseVal, CE);
-        break;
-      }
-      case Instruction::InsertElement: {
-        auto *Vec = CE->getOperand(0);
-        auto *Val = CE->getOperand(1);
-        addAssignEdge(Vec, CE);
-        addStoreEdge(Val, CE);
-        break;
-      }
-      case Instruction::ExtractElement: {
-        auto *Ptr = CE->getOperand(0);
-        addLoadEdge(Ptr, CE);
+        addAssignEdge(CE->getOperand(0), CE);
         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);
-        break;
-      }
-      case Instruction::ShuffleVector: {
-        auto *From1 = CE->getOperand(0);
-        auto *From2 = CE->getOperand(1);
-        addAssignEdge(From1, CE);
-        addAssignEdge(From2, CE);
+        addLoadEdge(CE->getOperand(0), 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:
+      case Instruction::Select:
+      case Instruction::ShuffleVector: {
         addAssignEdge(CE->getOperand(0), CE);
         addAssignEdge(CE->getOperand(1), CE);
         break;
+      }
 
       default:
         llvm_unreachable("Unknown instruction type encountered!");
       }
     }
   };
 
   // Helper functions