[PATCH] D51976: [DebugInfo][Dexter] Speculated BB presents illegal variable value to debugger

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso marked 7 inline comments as done.
CarlosAlbertoEnciso added inline comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:2528
+  findDbgUsers(DbgUsers, &I);
+  for (DbgVariableIntrinsic *DII : DbgUsers)
+    DII->eraseFromParent();
----------------
dblaikie wrote:
> CarlosAlbertoEnciso wrote:
> > CarlosAlbertoEnciso wrote:
> > > TWeaver wrote:
> > > > TWeaver wrote:
> > > > > I'd use for (auto &DII : DbgUsers) here. The type is obvious from the declaration of DbgUsers. This should make this line a little easier on the eyes.
> > > > is DII the correct name for the iterator here? should this be DVI inline with the name of the type used in the declaration of DbgUsers?
> > > > 
> > > > That is : DVI for DbgVariableIntrinsic.
> > > Changing the loop to `for (auto &DII : DbgUsers)`.
> > I can see your point.
> > 
> > The `SmallVector<DbgVariableIntrinsic *, 1>` is used in quite few other places and it seems the `DII` is the standard name used for the iterator.
> > 
> > I would prefer to follow that name convention.
> One tweak on this - if you're deducing a pointer with 'auto', LLVM style prefers to be explicit about that, so write "auto *X" rather than "auto &X" (assuming a reference isn't actually needed here, which it seems not)
@dblaikie,

I am sorry I missed your tweak. I am uploading a patch that includes it.



================
Comment at: test/CodeGen/X86/pr38763.ll:1
+; RUN: llc -O2 %s -o %t -filetype=obj
+; RUN: llvm-dwarfdump -debug-info %t | FileCheck %s
----------------
aprantl wrote:
> Sine you are testing SimplifyCFG, it would be much better to just run that one pass via opt and check the result immediately after it.
The new test case, run only the SimplifyCFG pass via opt.


================
Comment at: test/CodeGen/X86/pr38763.ll:7
+; When in the debugger, on the line "if (read == 4)", the value of "result"
+; is reported as '2', where it should be zero.
+
----------------
aprantl wrote:
> This is not a helpful description of the test. It would be easier to understand what the test is testing for if it said something like: "When SimplifyCFG folds two PHI instructions into a conditional instruction, the debug info becomes ambiguous..." (not sure if that is what actually happens but you get the idea.)
Updated test description.


https://reviews.llvm.org/D51976