SimplifyAllocas: search through more bitcasts to find alloca for llvm.dbg.declare.

SimplifyAllocas: search through more bitcasts to find alloca for llvm.dbg.declare.

The webp/snes9x build was generating more intermediate
bitcasts than expected, so the fixup for making
llvm.dbg.declare's first argument be an alloca was
sometimes failing to find the alloca. I.e., some earlier
pass like instcombine was already violating the contract
that llvm.dbg.declare's first argument be an alloca or a
function argument.

Try to be more resilient and loop through more casts if
needed.

Technically llvm.dbg.declare's first argument doesn't
*have* to be an alloca:

http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-June/086647.html

However, ReplacePtrsWithInts does not handle converting
llvm.dbg.declare parameters which are bitcasts, and ends
up erasing the bitcasts. This causes us to drop debug
info. An alternative is to fix up ReplacePtrsWithInts
(TODO), but we need to make that work with DCE.
That is, the conversions inserted by ReplacePtrsWithInts
must not be used solely by llvm.dbg.declare, otherwise
DCE will delete those conversions and leave us with
a null argument to llvm.dbg.declare.

Example program state prior to SimplifyAllocas:

; Function Attrs: nounwind readonly
define internal fastcc i32 @png_sig_cmp(i8* readonly %sig, i32 %start, i32 %num_to_check) #3 {
entry:
  %png_signature = alloca i64, align 8, !dbg !68085
  %tmpcast = bitcast i64* %png_signature to [8 x i8]*, !dbg !68085
  call void @llvm.dbg.declare(metadata [8 x i8]* %tmpcast, metadata !68092, metadata !45244), !dbg !68085
  store i64 727905341920923785, i64* %png_signature, align 8, !dbg !68085
  %cmp = icmp ugt i32 %num_to_check, 8, !dbg !68093
  br i1 %cmp, label %if.end3, label %if.else, !dbg !68095

// ...
if.end6:
// ...
  %gep_int6 = ptrtoint [8 x i8]* %tmpcast to i32, !dbg !68108
// ...


BUG= https://code.google.com/p/nativeclient/issues/detail?id=4203
R=dschuff@chromium.org

Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/7aaca4671623197b185445558109f20f883d66fa

[jvoung (off chromium), 2015-06-09 00:45:42]

jvoung@chromium.org changed reviewers:
+ dschuff@chromium.org

[jvoung (off chromium), 2015-06-09 01:07:08]

Hmm it seems like an earlier pass is already putting bitcasts in the 1st
argument.

Did you mean to find that earlier pass instead of patching this?

[Derek Schuff, 2015-06-09 16:01:12]

On 2015/06/09 01:07:08, jvoung wrote:
> Hmm it seems like an earlier pass is already putting bitcasts in the 1st
> argument.
> 
> Did you mean to find that earlier pass instead of patching this?

No, I hadn't realized there were other passes that did that, I had just noticed
that this pass was doing it and it broke debug info tests. The change I made to
SimplifyAllocas was just intended to keep it from breaking things; it would
probably be better to find where else this is happening rather than trying to
patch it up here. Maybe we could add something to the module verifier; I think
there is a way to make it run after every pass?

[jvoung (off chromium), 2015-06-10 21:58:20]

On 2015/06/09 16:01:12, Derek Schuff wrote:
> On 2015/06/09 01:07:08, jvoung wrote:
> > Hmm it seems like an earlier pass is already putting bitcasts in the 1st
> > argument.
> > 
> > Did you mean to find that earlier pass instead of patching this?
> 
> No, I hadn't realized there were other passes that did that, I had just
noticed
> that this pass was doing it and it broke debug info tests. The change I made
to
> SimplifyAllocas was just intended to keep it from breaking things; it would
> probably be better to find where else this is happening rather than trying to
> patch it up here. Maybe we could add something to the module verifier; I think
> there is a way to make it run after every pass?


Haven't settled on a fix, but here's an update.


So yes other passes can change the first arg of llvm.dbg.declare to be a
bitcast, and technically the first_arg in llvm.dbg.declare(first_arg) can be a
bitcast that leads to an alloca:

http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-June/086647.html


====

I tried reverting the various MV/VM patching in SimplifyAllocas to leave the
bitcasts alone, but I did repro the problems you had. It ends up breaking
"run_dwarf_local_var_objdump_test".


Watching the code after each pass, I notice that the ReplacePtrsWithInts pass
ends up


(a) not converting llvm.dbg.declare(metadata_ptr_arg) the way that it would
handle for example llvm.memcpy(ptr_arg), so metadata_ptr_arg still refers to a
bitcast.

then (b) a cleanup pass deletes bitcasts, assuming those are replaced by other
inserted conversions around loads/stores, etc. Thus the bitcast that the
metadata_ptr_arg is linked to gets deleted and the metadata becomes empty. Once
the metadata_ptr_arg is empty, DCE removes the llvm.dbg.declare.


If I patch ReplacePtrsWithInts to convert the llvm.dbg.declare(metadata_ptr_arg)
the way it converts llvm.memcpy(ptr_arg):

 --- a/lib/Transforms/NaCl/ReplacePtrsWithInts.cpp
+++ b/lib/Transforms/NaCl/ReplacePtrsWithInts.cpp
@@ -233,6 +233,18 @@ void FunctionConverter::convertInPlace(Instruction *Inst) {
     if (Arg->getType()->isPointerTy() && !ShouldLeaveAlone(Arg)) {
       Value *Conv = convert(Arg);
       Inst->setOperand(I, new IntToPtrInst(Conv, Arg->getType(), "", Inst));
+    } else if (auto *MV = dyn_cast<MetadataAsValue>(Arg)) {
+      if (auto *VM = dyn_cast<ValueAsMetadata>(MV->getMetadata())) {
+        if (VM->getValue()->getType()->isPointerTy()) {
+          Value *Conv = convert(VM->getValue());
+          Inst->setOperand(I, MetadataAsValue::get(
+              Inst->getContext(),
+              ValueAsMetadata::get(new IntToPtrInst(Conv,
+                                                    VM->getValue()->getType(),
+                                                    "",
+                                                    Inst))));
+        }
+      }
     }
   }

====

Then the dbg.declare survives that pass at least:

; Function Attrs: noinline nounwind uwtable
define hidden i32 @foo(i32 %dwarf_test_param) #0 {
entry:
  %dwarf_test_param.addr = alloca i8, i32 4, align 4
  %dwarf_test_param.addr.bc = bitcast i8* %dwarf_test_param.addr to i32*
  store i32 %dwarf_test_param, i32* %dwarf_test_param.addr.bc, align 4
  %dwarf_test_param.addr.bc1 = bitcast i8* %dwarf_test_param.addr to i32*
  call void @llvm.dbg.declare(metadata i32* %dwarf_test_param.addr.bc1, metadata
!1685, metadata !1686), !dbg !1687
  %dwarf_test_param.addr.bc2 = bitcast i8* %dwarf_test_param.addr to i32*
  %0 = load i32, i32* %dwarf_test_param.addr.bc2, align 4, !dbg !1688
  %call = call i32 @bar(i32 %0), !dbg !1689
  ret i32 %call, !dbg !1690
}



However that patch conjures up a fresh IntToPtr/Bitcast (.bc1), which is not
shared by other code like loads/stores which have their own fresh
IntToPtr/Bitcast (.bc and .bc2). Thus, the only user of the fresh cast is the
metadata_ptr_arg, but that doesn't count as a real "use" to DCE. So, the DCE
pass will then delete the parameter. One possibility is to try and share
IntToPtr/Bitcast with other "real" users like loads/stores. However, the
IntToPtr conversion will need to be placed at an insertion point that is after
the alloca yet dominates the both the load/store and the llvm.dbg.declare (so
.bc in the above case).


When you had the llvm.dbg.declare refer to the alloca instead, (a)
ReplacePtrsWithInts won't delete the alloca as it would a bitcast (assuming it's
subsumed by inserted ptrtoint and inttoptr) and (b) DCE will see other uses of
the alloca and also won't delete it.

[Derek Schuff, 2015-06-11 17:12:18]

I'm hitting what appears to be this problem when switching the IRT to a
full-debug build:
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...

[jvoung (off chromium), 2015-06-11 17:39:31]

On 2015/06/11 17:12:18, Derek Schuff wrote:
> I'm hitting what appears to be this problem when switching the IRT to a
> full-debug build:
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...

I imagine it could affect a bunch of programs with full debug-info and
optimization. Sam also saw this happening in the snes9x port.

I tried some other tweaks to the ReplacePtrsWithInts patch, but haven't gotten
it to fully work yet. I tried sharing/caching the inttoptrs based on
(def-of-int, insertion-point). Sharing was supposed to help make the use of the
bitcast not solely the llvm.dbg.declare to fight DCE. I tried to lift the
insertion-point of the shared inttoptr to that of the def instead of the use to
ensure dominance, but it needs to be smarter if the definition is a phi. Also
had some problem with dangling uses when erasing instructions.

Otherwise could try this patchup in SimplifyAllocas for now...

[Derek Schuff, 2015-06-11 17:43:12]

ok lets do this for now then.

https://codereview.chromium.org/1166253003/diff/1/lib/Transforms/NaCl/Simplif...
File lib/Transforms/NaCl/SimplifyAllocas.cpp (right):

https://codereview.chromium.org/1166253003/diff/1/lib/Transforms/NaCl/Simplif...
lib/Transforms/NaCl/SimplifyAllocas.cpp:130: ValueAsMetadata::get(Alloca)));
what will ValueAsMetadata do if you call get on nullptr?

[jvoung (off chromium), 2015-06-11 17:52:20]

https://codereview.chromium.org/1166253003/diff/1/lib/Transforms/NaCl/Simplif...
File lib/Transforms/NaCl/SimplifyAllocas.cpp (right):

https://codereview.chromium.org/1166253003/diff/1/lib/Transforms/NaCl/Simplif...
lib/Transforms/NaCl/SimplifyAllocas.cpp:130: ValueAsMetadata::get(Alloca)));
On 2015/06/11 17:43:12, Derek Schuff wrote:
> what will ValueAsMetadata do if you call get on nullptr?

It'll assert or deref null. I'll change the findAllocaFromBC to
report_fatal_error instead of returning nullptr?

[Derek Schuff, 2015-06-11 17:53:06]

lgtm

[jvoung (off chromium), 2015-06-11 19:48:06]

Committed patchset #2 (id:20001) manually as
7aaca4671623197b185445558109f20f883d66fa (presubmit successful).