[Polly] Fix code generation of llvm.expect intrinsic

[annanay25]

Summary

At the time of code generation, an instruction with an llvm intrinsic is ignored in copyBB. However, if the value of the instruction is used later in the program, the value needs to be synthesized. However, this is causing some issues with the instructions being generated in a hoisted basic block.

Removing llvm.expect from the list of ignored intrinsics fixes this bug.

[grosser]

Hi Annay,

this change should include a test case.

[annanay25]

Hi sir,

Actually, I was trying to incorporate the changes that Johannes sir suggested yesterday. I have temporarily made these changes, specifically for the llvm.expect intrinsic (it can be later extended to other intrinsics) but it seems to be causing some side effects. Specifically, the llvm.expect instruction is correctly code generated with this change, but the trunc statement following that is not. Will update shortly sir.

https://gist.github.com/annanay25/5c4f47ab96cba1f0220f49b455fb191b#file-tempblockgenerators-cpp-L9-L13

[annanay25]

Incorporated Johannes sir's suggestion. Works now.

Basically, changed the instruction to be code generated from

%e = llvm.expect(%f, ..)

to

%e = %f

BBMap should continue to be in VTV in trySynthesizeNewValue().
@grosser sir, if you could confirm this, I will remove the [WIP] and add a minimal test case.

[Meinersbur]

No need to remove the llvm.expect globally. You can just skip it using BBMap:

BBMap[%e] = %f

The other possibility is to remove llvm.expect from isIgnoredIntrinsic. Polly does not change the value of %f, so it can be argued that the generated value is still the same.

[annanay25]

Fixes code generation of llvm.expect clause.

By removing llvm.expect from the list of ignored intrinsics, we generate code for it in copyBB like a normal instruction. This mitigates the problem of synthesizing the statement along with all its dependences in a hoisted basic block.

[grosser]

Hi Annanay,

instead of commenting out the function, can you remove it. Also, can you please add a test case.

[grosser]

Great. Looks good. Can you also update the commit message to summarize the discussions we had and to explain why we choose the solution now implemented.

polly/trunk/lib/Support/ScopHelper.cpp

if (auto *IT = dyn_cast<IntrinsicInst>(V)) {
     case llvm::Intrinsic::invariant_start:
     case llvm::Intrinsic::invariant_end:
     // Some misc annotations are supported/ignored.
     case llvm::Intrinsic::var_annotation:
     case llvm::Intrinsic::ptr_annotation:
     case llvm::Intrinsic::annotation:
     case llvm::Intrinsic::donothing:
     case llvm::Intrinsic::assume:
-    case llvm::Intrinsic::expect:
     // Some debug info intrisics are supported/ignored.
     case llvm::Intrinsic::dbg_value:
     case llvm::Intrinsic::dbg_declare:
       return true;
     default:
       break;
     }
   }

polly/trunk/test/CodeGen/Intrinsics/llvm-expect.ll

+; RUN: opt %loadPolly -S -polly-codegen < %s | FileCheck %s
+;
+; Check that we generate code without crashing.
+;
+; CHECK: polly.start
+;
+target datalayout = "e-p:64:64:64-S128-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f16:16:16-f32:32:32-f64:64:64-f128:128:128-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
+
+; Function Attrs: nounwind uwtable
+define void @quux() unnamed_addr #0 {
+bb:
+  br label %bb1
+
+bb1:                                              ; preds = %bb
+  %tmp = icmp eq i64 0, 0
+  br i1 %tmp, label %bb2, label %bb7
+
+bb2:                                              ; preds = %bb1
+  %tmp3 = icmp ult i64 4, 12
+  %tmp4 = zext i1 %tmp3 to i64
+  %tmp5 = tail call i64 @llvm.expect.i64(i64 %tmp4, i64 0)
+  %tmp6 = trunc i64 %tmp5 to i32
+  br label %bb7
+
+bb7:                                              ; preds = %bb2, %bb1
+  %tmp8 = phi i32 [ undef, %bb2 ], [ 0, %bb1 ]
+  ret void
+}
+
+; Function Attrs: nounwind readnone
+declare i64 @llvm.expect.i64(i64, i64) #1
+
+attributes #0 = { nounwind uwtable }
+attributes #1 = { nounwind readnone }