[CFLGraph] Fixed Select instruction handling

[xbolva00]

Summary

Operand 0 is the condition, not the true value.

Use op 1 and op 2 as the correct values.

[lebedev.ri]

Tests?

[xbolva00]

Added test case

[xbolva00]

Can you check even this patch, @rjmccall , please?

This old bug was found by @efriedma https://reviews.llvm.org/D46259

[rjmccall]

I don't know why you're CC'ing me on these patches in LLVM code I don't know anything about, and I think if I keep reviewing them you're going to keep doing it, and I don't have time to review a bunch of miscellaneous LLVM patches, sorry.

[craig.topper]

test/Analysis/CFLAliasAnalysis/Andersen/select.ll

1 ↗

(On Diff #144845)

The checks were definitely not generated by this script.

7 ↗

(On Diff #144845)

This passes on trunk already.

[xbolva00]

Can you recommend me the right test?

Something like print cfl graph?

[craig.topper]

I have no idea this isn't an area I'm familar with. The code is for ConstantExpr selects, but the select you created isn't a ConstantExpr. Almost the entire visitConstantExpr function is showing no coverage on the coverage bots http://llvm.org/reports/coverage/lib/Analysis/CFLGraph.h.gcov.html so I don't even know what to do.

[xbolva00]

In D46343#1086840, @rjmccall wrote:

I don't know why you're CC'ing me on these patches in LLVM code I don't know anything about, and I think if I keep reviewing them you're going to keep doing it, and I don't have time to review a bunch of miscellaneous LLVM patches, sorry.

Sorry for it

[xbolva00]

In D46343#1086890, @craig.topper wrote:

I have no idea this isn't an area I'm familar with. The code is for ConstantExpr selects, but the select you created isn't a ConstantExpr. Almost the entire visitConstantExpr function is showing no coverage on the coverage bots http://llvm.org/reports/coverage/lib/Analysis/CFLGraph.h.gcov.html so I don't even know what to do.

I will try to rework it to ConstantExpr..

[xbolva00]

test/Analysis/CFLAliasAnalysis/Andersen/select.ll

1 ↗

(On Diff #144845)

I ran the script but somehow failed to generate "CHECK" lines for me. I did them myself manually ...

[xbolva00]

Sorry, I tried to make test case as:

@a = common dso_local global i32 0, align 4
@b = common dso_local global i32 0, align 4

; Function Attrs: norecurse nounwind readnone uwtable
define void @test_select() local_unnamed_addr #0 {
entry:

select i1 false, i32* @b, i32* @a
ret void

}

But I got same output with and without patch :(

Since I am unable to create proper test case, I will just remove current test and leave this as is - whether you accept it without patch or we can leave it buggy..

[xbolva00]

Can this be merged even without test case, @craig.topper ?

[george.burgess.iv]

Hi, and thanks for the patch! I'm pretty familiar with this code and am happy to review it when I get back from vacation next week.

[xbolva00]

In D46343#1088900, @george.burgess.iv wrote:

Hi, and thanks for the patch! I'm pretty familiar with this code and am happy to review it when I get back from vacation next week.

Thanks.

Enjoy your vacation! :)

[george.burgess.iv]

LGTM; thanks again.

I have a test-case I'll commit as a follow-up for this patch.

[george.burgess.iv]

(Test committed as r332017)

lib/Analysis/CFLGraph.h

void visitConstantExpr(ConstantExpr *CE) {
       case Instruction::UIToFP:
       case Instruction::SIToFP:
       case Instruction::FPToUI:
       case Instruction::FPToSI: {
         addAssignEdge(CE->getOperand(0), CE);
         break;
       }
 
+      case Instruction::Select: {
+        addAssignEdge(CE->getOperand(1), CE);
+        addAssignEdge(CE->getOperand(2), CE);
+        break;
+      }
+
       case Instruction::InsertElement:
       case Instruction::InsertValue: {
         addAssignEdge(CE->getOperand(0), CE);
         addStoreEdge(CE->getOperand(1), CE);
         break;
       }
 
       case Instruction::ExtractElement:
void visitConstantExpr(ConstantExpr *CE) {
       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!");