Page MenuHomePhabricator

[CGP] Drop no op intrinsic calls
AbandonedPublic

Authored by xbolva00 on Aug 16 2019, 2:57 AM.

Details

Summary

If llvm.assume expression contains side effect, Clang doesn't codegen such llvm.assume. Otherwise, if the expr is "pure/const", it may survive the whole middlend pipeline.
The problem: AFAIK llvm.assume calls are not propagated to backend. This means the backend just drops it, but fails to eliminate dead instructions related to llvm.assumes.

Should fix PR43010.

Diff Detail

Event Timeline

xbolva00 created this revision.Aug 16 2019, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2019, 2:57 AM

(As an alternative, this can be fixed in the phrase when llvm.assume is dropped in backend.. but not sure if we have "backend" version of RecursivelyDeleteTriviallyDeadInstructions)

xbolva00 updated this revision to Diff 215613.EditedAug 16 2019, 8:56 AM

Oh, this is funny (or nasty) bug.. Never called function is now called! :)

a.c
int foo(void) attribute((const));

int main(void) {

__builtin_assume(foo() == 1);

}

b.c
#include <stdio.h>

int foo(void) {

puts("BUM");
return 0;

}

clang -O3 a.c b.c -o bum

./bum
BUM

Side note: As shown in this example, attribute((const)) on declarations seems like a bad idea. User may add attribute((const)), then in the future introduce a side effect aaaand... compiler is happy, optimizers are happy, no warnings, weird binary. Can clang-tidy catch it? @aaron.ballman @NoQ GCC folks claim this is hard to do and say attribute((const)) is a best effort thing - you simply shouldn't do a mistake.

xbolva00 added a subscriber: NoQ.Aug 16 2019, 9:05 AM

Oh, this is funny (or nasty) bug.. Never called function is now called! :)

a.c
int foo(void) attribute((const));

int main(void) {

__builtin_assume(foo() == 1);

}

b.c
#include <stdio.h>

int foo(void) {

puts("BUM");
return 0;

}

clang -O3 a.c b.c -o bum

./bum
BUM

Side note: As shown in this example, attribute((const)) on declarations seems like a bad idea. User may add attribute((const)), then in the future introduce a side effect aaaand... compiler is happy, optimizers are happy, no warnings, weird binary. Can clang-tidy catch it? @aaron.ballman @NoQ GCC folks claim this is hard to do and say attribute((const)) is a best effort thing - you simply shouldn't do a mistake.

This is not a bug. It's user error. We certainly might have better tooling to catch these kinds of errors, of course.

Anyway, this patch is still valid to be reviewed.

Original PR does not mention const-mismatch (just my experiment).

(As an alternative, this can be fixed in the phrase when llvm.assume is dropped in backend.. but not sure if we have "backend" version of RecursivelyDeleteTriviallyDeadInstructions)

When I originally looked at this, dropping the assume during SelectionDAG construction seemed sufficient to also drop side-effect-free instructions used only by the assume. Maybe this SDAG logic should be extended to drop readnone functions in the same way?

However, what I don't know is: In the context of GlobalISel (and maybe also FastISel) we really need to actively deal with this in some other way (e.g., like this)? Thoughts?

(As an alternative, this can be fixed in the phrase when llvm.assume is dropped in backend.. but not sure if we have "backend" version of RecursivelyDeleteTriviallyDeadInstructions)

When I originally looked at this, dropping the assume during SelectionDAG construction seemed sufficient to also drop side-effect-free instructions used only by the assume. Maybe this SDAG logic should be extended to drop readnone functions in the same way?

However, what I don't know is: In the context of GlobalISel (and maybe also FastISel) we really need to actively deal with this in some other way (e.g., like this)? Thoughts?

For GlobalISel side effect free dead instructions will be deleted at various points in the pipeline. However, calls won't be. I don't think clang should be allowing this at all though. It should be perfectly legal to drop an llvm.assume without having to deal with dead defs.

NoQ added a comment.Aug 16 2019, 1:14 PM

Side note: As shown in this example, attribute((const)) on declarations seems like a bad idea. User may add attribute((const)), then in the future introduce a side effect aaaand... compiler is happy, optimizers are happy, no warnings, weird binary. Can clang-tidy catch it? @aaron.ballman @NoQ GCC folks claim this is hard to do and say attribute((const)) is a best effort thing - you simply shouldn't do a mistake.

The Static Analyzer could be taught to catch it (unreliably - it's not a verifier!) as long as the definition of a side effect is precise enough ("doesn't write into global variables" is precise enough, "causes no observable change in program behavior" is not precise enough). I'm generally a big fan of having better purity/constness annotations but these current imprecisions are regularly driving me nuts. Clang-Tidy could catch a subset of cases syntactically (eg., it could catch all writes into global variables as long as they happen within the same function, but finding such bugs in a nested function call will be impossible without involving path-sensitive analysis or suffering from a certain class of false positives).

I was thinking about a way to support generic assumes, e.g., they can have side-effects, etc. I'll submit a design RFC soon.

Anyway I think this fix is good and simple - does not require individual fixes for GlobalISel, FastISel, SelectionDAG.

Anyway I think this fix is good and simple - does not require individual fixes for GlobalISel, FastISel, SelectionDAG.

Indeed. But this situation is not unique to assume. The relevant code in SDAGBuilder has:

case Intrinsic::assume:
case Intrinsic::var_annotation:
case Intrinsic::sideeffect:
  // Discard annotate attributes, assumptions, and artificial side-effects.
  return;

can we drop all of them at this point, or is it only safe to drop assumes there?

nikic added a subscriber: nikic.Aug 17 2019, 9:23 AM
nikic added inline comments.
lib/CodeGen/CodeGenPrepare.cpp
6941

Move into optimizeCallInst, where other intrinsics are handled?

xbolva00 updated this revision to Diff 215747.Aug 17 2019, 9:32 AM
xbolva00 retitled this revision from [CGP] Drop llvm.assume calls to [CGP] Drop no op intrinsic calls.

Moved to optimizeCallInst.
Drop var.annotation and sideeffect intrinsics too.

xbolva00 marked 2 inline comments as done.Aug 17 2019, 9:33 AM
xbolva00 added inline comments.
lib/CodeGen/CodeGenPrepare.cpp
6941

+1, thanks for tip.

xbolva00 edited the summary of this revision. (Show Details)Aug 17 2019, 9:36 AM
xbolva00 marked an inline comment as done.Fri, Aug 23, 3:01 PM

Ping

hfinkel added inline comments.Thu, Aug 29, 4:40 PM
lib/CodeGen/CodeGenPrepare.cpp
1856

The others look fine to me (although, honestly, I don't understand how var_annotation is really used), but dropping llvm::sideeffect in CGP makes me really nervous. This is necessary for correctness in the places where it's used, and could lead to invalid analysis/transformation results in post-CGP IR passes/analysis. I recommend not doing this here.

bjope added a subscriber: bjope.Thu, Aug 29, 10:44 PM

(just sharing my thoughts after seeing this patch)

Is CGP a "mandatory" pass always expected to be executed before ISel? In other words, can we remove logic from SelectionDAGBuilder when rewriting intrinsics here (simply assert that we no longer find intrinsics that should be eliminated by CGP)?

Is it allowed to add passes between CGP and ISel? In other words, could there for example be forks of LLVM that is using llvm.var.annotation between CGP and ISel? Then perhaps we need to describe this strategy better in some way (LangRef isn't that precise in telling when the intrinsics are removed right now, and I doubt it would be much details mentioning exactly when it happens in the LangRef, but I guess one would assume that it hangs around until ISel. But maybe CodegenPrepare should be seen more or less as part of ISel (such as being mandatory before SelectionDAGBuilder).

If we rewrite llvm.var.annotation in CGP, then I guess we shoud rewrite llvm.ptr.annotation and llvm.annotation as well?
To me it seems a little bit strange not to eliminate these at the same point in time. Although, unless we also can remove logic for handling these in ISel it just adds the logic for how to eliminate them one more time.

Some day I hope the pass description for CGP is updated. It is still saying that the pass "should eventually be removed.", but still we add more and more stuff here.

Is CGP a "mandatory" pass always expected to be executed before ISel? In other words, can we remove logic from SelectionDAGBuilder when rewriting intrinsics here (simply assert that we no longer find intrinsics that should be eliminated by CGP)?

No and no.

Is it allowed to add passes between CGP and ISel?

A target could certainly do that. This is a good point.

Some day I hope the pass description for CGP is updated. It is still saying that the pass "should eventually be removed.", but still we add more and more stuff here.

I believe that the thought is that we'll be able to do this once we have GlobalISel.

xbolva00 added a comment.EditedFri, Aug 30, 8:46 AM

probably not so good to do it in CGP then..

xbolva00 abandoned this revision.Fri, Aug 30, 8:48 AM