[CGP] Drop no op intrinsic calls

[xbolva00]

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.

[xbolva00]

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

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.

[hfinkel]

In D66338#1633249, @xbolva00 wrote:

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.

[xbolva00]

Anyway, this patch is still valid to be reviewed.

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

[hfinkel]

In D66338#1633098, @xbolva00 wrote:

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

[aemerson]

In D66338#1633517, @hfinkel wrote:

In D66338#1633098, @xbolva00 wrote:

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

In D66338#1633249, @xbolva00 wrote:

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

[jdoerfert]

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.

[xbolva00]

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

[hfinkel]

In D66338#1634061, @xbolva00 wrote:

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]

lib/CodeGen/CodeGenPrepare.cpp

6941

Move into optimizeCallInst, where other intrinsics are handled?

[xbolva00]

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

[xbolva00]

lib/CodeGen/CodeGenPrepare.cpp

6941

+1, thanks for tip.

[xbolva00]

Ping

[xbolva00]

Ping

[hfinkel]

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]

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

[hfinkel]

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]

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

lib/CodeGen/CodeGenPrepare.cpp

for (auto &Arg : CI->arg_operands()) {
       unsigned AS = Arg->getType()->getPointerAddressSpace();
       return optimizeMemoryInst(CI, Arg, Arg->getType(), AS);
     }
 
   IntrinsicInst *II = dyn_cast<IntrinsicInst>(CI);
   if (II) {
     switch (II->getIntrinsicID()) {
     default: break;
+    case Intrinsic::sideeffect: {
+      II->eraseFromParent();
+      return true;
+    }
+    case Intrinsic::assume:
+    case Intrinsic::var_annotation: {
+      Value *Arg = II->getOperand(0);
+      II->eraseFromParent();
+      RecursivelyDeleteTriviallyDeadInstructions(Arg);
+      return true;
+    }
     case Intrinsic::experimental_widenable_condition: {
       // Give up on future widening oppurtunties so that we can fold away dead
       // paths and merge blocks before going into block-local instruction
       // selection.   
       if (II->use_empty()) {
         II->eraseFromParent();
         return true;
       }

 
 bool CodeGenPrepare::optimizeInst(Instruction *I, bool &ModifiedDT) {
   // Bail out if we inserted the instruction to prevent optimizations from
   // stepping on each other's toes.
   if (InsertedInsts.count(I))
     return false;
 
   // TODO: Move into the switch on opcode below here.
   if (PHINode *P = dyn_cast<PHINode>(I)) {
     // It is possible for very late stage optimizations (such as SimplifyCFG)
     // to introduce PHI nodes too late to be cleaned up.  If we detect such a
     // trivial PHI, go ahead and zap it here.
     if (Value *V = SimplifyInstruction(P, {*DL, TLInfo})) {
       LargeOffsetGEPMap.erase(P);
       P->replaceAllUsesWith(V);
       P->eraseFromParent();
       ++NumPHIsElim;

test/Transforms/CodeGenPrepare/drop-intrinsics.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -codegenprepare  -S %s | FileCheck %s
+
+declare i32 @foo()
+declare i8* @bar()
+declare void @llvm.assume(i1)
+declare void @llvm.sideeffect()
+declare void @llvm.var.annotation(i8*, i8*, i8*, i32)
+
+define void @drop_llvm_assume() {
+; CHECK-LABEL: @drop_llvm_assume(
+; CHECK-NEXT:    ret void
+;
+  %call = tail call i32 @foo() #0
+  %cmp = icmp eq i32 %call, 1
+  call void @llvm.assume(i1 %cmp)
+  ret void
+}
+
+define void @drop_llvm_sideeffect() {
+; CHECK-LABEL: @drop_llvm_sideeffect(
+; CHECK-NEXT:    ret void
+;
+  call void @llvm.sideeffect()
+  ret void
+}
+
+define void @drop_llvm_var_annotation() {
+; CHECK-LABEL: @drop_llvm_var_annotation(
+; CHECK-NEXT:    ret void
+;
+  %s = call i8* @bar() #0
+  call void @llvm.var.annotation(i8* %s, i8* null, i8* null, i32 0)
+  ret void
+}
+
+attributes #0 = { nounwind readnone }