Page MenuHomePhabricator

[CFLGraph] Fixed Select instruction handling
ClosedPublic

Authored by xbolva00 on May 1 2018, 6:23 PM.

Details

Summary

Operand 0 is the condition, not the true value.

Use op 1 and op 2 as the correct values.

Diff Detail

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.May 1 2018, 6:23 PM
xbolva00 edited the summary of this revision. (Show Details)May 1 2018, 6:29 PM
xbolva00 updated this revision to Diff 144845.May 2 2018, 3:29 AM

Added test case

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

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

efriedma edited reviewers, added: george.burgess.iv; removed: efriedma.May 3 2018, 10:58 AM

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 added inline comments.
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 added a comment.EditedMay 3 2018, 12:13 PM

Can you recommend me the right test?

Something like print cfl graph?

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 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

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 added inline comments.May 3 2018, 12:30 PM
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 added a comment.EditedMay 3 2018, 4:08 PM

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 updated this revision to Diff 145112.May 3 2018, 4:09 PM

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

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.

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! :)

LGTM; thanks again.

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

This revision is now accepted and ready to land.May 10 2018, 12:20 AM
This revision was automatically updated to reflect the committed changes.

(Test committed as r332017)