This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Recognize simple br-phi patterns
ClosedPublic

Authored by sanjoy on Oct 2 2015, 1:34 AM.

Details

Summary

Teach SCEV to match patterns like

 br %cond, label %left, label %right
left:
 br label %merge
right:
 br label %merge
merge:
 V = phi [ %x, %left ], [ %y, %right ]

as "select %cond, %x, %y". Before this SCEV would match PHI nodes
exclusively to add recurrences.

This addresses PR25005.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 36335.Oct 2 2015, 1:34 AM
sanjoy retitled this revision from to [SCEV] Recognize simple br-phi patterns.
sanjoy updated this object.
sanjoy added a subscriber: llvm-commits.
atrick accepted this revision.Oct 2 2015, 8:26 AM
atrick edited edge metadata.

Nice! Thanks Sanjoy.

This revision is now accepted and ready to land.Oct 2 2015, 8:26 AM
mehdi_amini edited edge metadata.Oct 2 2015, 1:32 PM

Hi Sanjoyd,

Thanks for the clarification on the Constant Select, isn't it something that could be handled as well? Especially with D13390 coming?
See inline some other comments.

Thanks!

lib/Analysis/ScalarEvolution.cpp
3601 ↗(On Diff #36335)

Update the comment

3781 ↗(On Diff #36335)

I have a question about the lambda and their size, and especially the second one with this inner structure. Usually I have seen in LLVM only very small lambda used in favor of static function for larger one. Also Structure helper are usually in an anonymous namespace. Is there any guideline on this?

3872 ↗(On Diff #36335)

What about renaming createNodeForSelect into createNodeForSelectOrPhi? And also update the comment on top of the definition for createNodeForSelect.

sanjoy added inline comments.Oct 2 2015, 1:58 PM
lib/Analysis/ScalarEvolution.cpp
3781 ↗(On Diff #36335)

First of all, I'm not opposed to moving lifting the lambda outside if you think that'll help readability. I don't remember seeing anything about this in the coding style, but I can certainly understand the sentiment. :)

My bias is towards keeping things in as restricted a scope as possible. If a structure / helper function is useful only inside a function, then it should stay within that function, and not be leaked into the file scope etc. IOW, if something is not generally useful, it should be generally available either (same reason why you'd want to put the struct in an anonymous namespace, and not inside the llvm namespace). But this is just a bias (not a strong rational reason).

I don't have a definitive opinion on the topic, but I did the same as you do in the past, and for the same reason ;), but I was asked to change it I think.

mehdi_amini edited edge metadata.Oct 2 2015, 2:02 PM
mehdi_amini added a subscriber: dexonsmith.
sanjoy added a comment.Oct 2 2015, 2:02 PM
In D13378#258915, @joker.eph wrote:

Hi Sanjoyd,

Thanks for the clarification on the Constant Select, isn't it something that could be handled as well? Especially with D13390 coming?

I'm not sure there is much value in optimizing ConstantExprs in SCEV, because they're compile time constants anyway (though they may need to remain symbolic till link time). I'd just wait till we see a case where it matters in practice.

See inline some other comments.

Thanks!

sanjoy updated this revision to Diff 36400.Oct 2 2015, 2:59 PM
  • address review: I've split createNodeForPHI into two subroutines, and removed the misleading comment. Also renamed createNodeFromSelect.
sanjoy updated this revision to Diff 36401.Oct 2 2015, 3:00 PM
  • use slash (///) in header comments
This revision was automatically updated to reflect the committed changes.

Sanjoy,

This commit is causing an internal compiler error with the following code:

int x0, x1, x2;
double *x3;
int fsolv() {

int x4, x5 = x4 = 0;
for (; x4 < x2; ++x4) {
  x0 = 0;
  for (; x0 < x4; ++x0)
    x1 = x3[x5++];
}
return 0;

}

Is this something you recognize? If not, do you mind if I revert your
commit while you work on the test case?

Thanks. Diego.

$ clang -c -O2 a.c
clang-3.8:
/usr/local/google/home/dnovillo/llvm/llvm/lib/Analysis/LazyValueInfo.cpp:632:
bool (anonymous namespace)::LazyValueInfoCache
::solveBlockValueNonLocal((anonymous namespace)::LVILatticeVal &,
llvm::Value *, llvm::BasicBlock *): Assertion `isa<Argument>(Val) &&
"Unknown live-in to the entry block"' failed.
0 libLLVMSupport.so.3.8 0x00007f484d2fafde
llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 46
1 libLLVMSupport.so.3.8 0x00007f484d2fc119
2 libLLVMSupport.so.3.8 0x00007f484d2fab53
llvm::sys::RunSignalHandlers() + 131
3 libLLVMSupport.so.3.8 0x00007f484d2fc35b
4 libc.so.6 0x00007f484a447d40
5 libc.so.6 0x00007f484a447cc9 gsignal + 57
6 libc.so.6 0x00007f484a44b0d8 abort + 328
7 libc.so.6 0x00007f484a440b86
8 libc.so.6 0x00007f484a440c32
9 libLLVMAnalysis.so.3.8 0x00007f48504ba546
10 libLLVMAnalysis.so.3.8 0x00007f48504b9dd6
11 libLLVMAnalysis.so.3.8 0x00007f48504b97b2
12 libLLVMAnalysis.so.3.8 0x00007f48504b776e
13 libLLVMAnalysis.so.3.8 0x00007f48504b7541
llvm::LazyValueInfo::getConstantOnEdge(llvm::Value*, llvm::BasicBlock*,
llvm::BasicBl
ock*, llvm::Instruction*) + 129
14 libLLVMScalarOpts.so.3.8 0x00007f484d60a6ea
15 libLLVMScalarOpts.so.3.8 0x00007f484d60a317
16 libLLVMCore.so.3.8 0x00007f484f4281ff
llvm::FPPassManager::runOnFunction(llvm::Function&) + 399
17 libLLVMAnalysis.so.3.8 0x00007f48503f1611
18 libLLVMAnalysis.so.3.8 0x00007f48503f107d
19 libLLVMAnalysis.so.3.8 0x00007f48503f0928
20 libLLVMCore.so.3.8 0x00007f484f428caa
21 libLLVMCore.so.3.8 0x00007f484f4287d6
llvm::legacy::PassManagerImpl::run(llvm::Module&) + 342
22 libLLVMCore.so.3.8 0x00007f484f4291c1
llvm::legacy::PassManager::run(llvm::Module&) + 33
23 libclangCodeGen.so.3.8 0x00007f484bbb6dd5
24 libclangCodeGen.so.3.8 0x00007f484bbb6502
clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::CodeGenOptions
const&, clang::TargetOptions const&, clang::LangOptions const&,
llvm::StringRef, llvm::Module*, clang::BackendAction,
llvm::raw_pwrite_stream*) + 162
25 libclangCodeGen.so.3.8 0x00007f484be89a39
26 libclangParse.so.3.8 0x00007f48463905b1
clang::ParseAST(clang::Sema&, bool, bool) + 817
27 libclangFrontend.so.3.8 0x00007f484b402d10
clang::ASTFrontendAction::ExecuteAction() + 320
28 libclangCodeGen.so.3.8 0x00007f484be875f4
clang::CodeGenAction::ExecuteAction() + 2100
29 libclangFrontend.so.3.8 0x00007f484b4027f0
clang::FrontendAction::Execute() + 112
30 libclangFrontend.so.3.8 0x00007f484b39f402
clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 978
31 libclangFrontendTool.so.3.8 0x00007f484affa764
clang::ExecuteCompilerInvocation(clang::CompilerInstance*) + 1172
32 clang-3.8 0x0000000000423f08
cc1_main(llvm::ArrayRef<char const*>, char const*, void*) + 920
33 clang-3.8 0x0000000000417839
34 clang-3.8 0x0000000000416715 main + 2277
35 libc.so.6 0x00007f484a432ec5 __libc_start_main + 245
36 clang-3.8 0x0000000000415ce4
Stack dump:

  1. <eof> parser at end of file
  2. Per-module optimization passes
  3. Running pass 'CallGraph Pass Manager' on module 'a.c'.
  4. Running pass 'Value Propagation' on function '@fsolv'

Diego Novillo via llvm-commits wrote:

dnovillo added a subscriber: dnovillo.
dnovillo added a comment.

Sanjoy,

This commit is causing an internal compiler error with the following code:

int x0, x1, x2;
double *x3;
int fsolv() {

int x4, x5 = x4 = 0;
for (; x4<  x2; ++x4) {
  x0 = 0;
  for (; x0<  x4; ++x0)
    x1 = x3[x5++];
}
return 0;

}

Is this something you recognize? If not, do you mind if I revert your
commit while you work on the test case?

It is plausible that this is the same issue as in http://reviews.llvm.org/D13460. If it is possible, can you please try
D13460 and see if it fixes the ICE? If not or if trying out a new patch will be too much work, reverting this is okay.

Thanks!

  • Sanjoy

Thanks. Diego.

$ clang -c -O2 a.c
clang-3.8:
/usr/local/google/home/dnovillo/llvm/llvm/lib/Analysis/LazyValueInfo.cpp:632:
bool (anonymous namespace)::LazyValueInfoCache
::solveBlockValueNonLocal((anonymous namespace)::LVILatticeVal&,
llvm::Value *, llvm::BasicBlock *): Assertion `isa<Argument>(Val)&&
"Unknown live-in to the entry block"' failed.
0 libLLVMSupport.so.3.8 0x00007f484d2fafde
llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 46
1 libLLVMSupport.so.3.8 0x00007f484d2fc119
2 libLLVMSupport.so.3.8 0x00007f484d2fab53
llvm::sys::RunSignalHandlers() + 131
3 libLLVMSupport.so.3.8 0x00007f484d2fc35b
4 libc.so.6 0x00007f484a447d40
5 libc.so.6 0x00007f484a447cc9 gsignal + 57
6 libc.so.6 0x00007f484a44b0d8 abort + 328
7 libc.so.6 0x00007f484a440b86
8 libc.so.6 0x00007f484a440c32
9 libLLVMAnalysis.so.3.8 0x00007f48504ba546
10 libLLVMAnalysis.so.3.8 0x00007f48504b9dd6
11 libLLVMAnalysis.so.3.8 0x00007f48504b97b2
12 libLLVMAnalysis.so.3.8 0x00007f48504b776e
13 libLLVMAnalysis.so.3.8 0x00007f48504b7541
llvm::LazyValueInfo::getConstantOnEdge(llvm::Value*, llvm::BasicBlock*,
llvm::BasicBl
ock*, llvm::Instruction*) + 129
14 libLLVMScalarOpts.so.3.8 0x00007f484d60a6ea
15 libLLVMScalarOpts.so.3.8 0x00007f484d60a317
16 libLLVMCore.so.3.8 0x00007f484f4281ff
llvm::FPPassManager::runOnFunction(llvm::Function&) + 399
17 libLLVMAnalysis.so.3.8 0x00007f48503f1611
18 libLLVMAnalysis.so.3.8 0x00007f48503f107d
19 libLLVMAnalysis.so.3.8 0x00007f48503f0928
20 libLLVMCore.so.3.8 0x00007f484f428caa
21 libLLVMCore.so.3.8 0x00007f484f4287d6
llvm::legacy::PassManagerImpl::run(llvm::Module&) + 342
22 libLLVMCore.so.3.8 0x00007f484f4291c1
llvm::legacy::PassManager::run(llvm::Module&) + 33
23 libclangCodeGen.so.3.8 0x00007f484bbb6dd5
24 libclangCodeGen.so.3.8 0x00007f484bbb6502
clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::CodeGenOptions
const&, clang::TargetOptions const&, clang::LangOptions const&,
llvm::StringRef, llvm::Module*, clang::BackendAction,
llvm::raw_pwrite_stream*) + 162
25 libclangCodeGen.so.3.8 0x00007f484be89a39
26 libclangParse.so.3.8 0x00007f48463905b1
clang::ParseAST(clang::Sema&, bool, bool) + 817
27 libclangFrontend.so.3.8 0x00007f484b402d10
clang::ASTFrontendAction::ExecuteAction() + 320
28 libclangCodeGen.so.3.8 0x00007f484be875f4
clang::CodeGenAction::ExecuteAction() + 2100
29 libclangFrontend.so.3.8 0x00007f484b4027f0
clang::FrontendAction::Execute() + 112
30 libclangFrontend.so.3.8 0x00007f484b39f402
clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 978
31 libclangFrontendTool.so.3.8 0x00007f484affa764
clang::ExecuteCompilerInvocation(clang::CompilerInstance*) + 1172
32 clang-3.8 0x0000000000423f08
cc1_main(llvm::ArrayRef<char const*>, char const*, void*) + 920
33 clang-3.8 0x0000000000417839
34 clang-3.8 0x0000000000416715 main + 2277
35 libc.so.6 0x00007f484a432ec5 __libc_start_main + 245
36 clang-3.8 0x0000000000415ce4
Stack dump:

1.<eof> parser at end of file

  1. Per-module optimization passes
  2. Running pass 'CallGraph Pass Manager' on module 'a.c'.
  3. Running pass 'Value Propagation' on function '@fsolv'

Repository:

rL LLVM

http://reviews.llvm.org/D13378


llvm-commits mailing list
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits