Page MenuHomePhabricator

markus (Markus Lavin)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 19 2018, 1:12 AM (13 w, 5 d)

Recent Activity

Yesterday

markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Fri, Feb 22, 5:08 AM · debug-info
markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.
  • Added test with two CUs.
  • Updated some comments.
  • Removed the -generate-typed-dwarf5-expr option, will only produce DW_OP_convert for Dwarf v5, for all prior versions the mask & shift expression is used. I suggest that we forget about DW_OP_GNU_convert and debugger tunings for this review. If that is really desired it can be added later in a separate patch.
Fri, Feb 22, 4:52 AM · debug-info

Thu, Feb 21

markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.
  • updated to top of trunk to have the llvm-dwarfdump updates committed earlier today
  • updated tests and made additional improvements to llvm-dwarfdump
  • fixed segfault bug where we crashed on release builds but not debug builds (the allocation of DIEBaseTypeRef)
Thu, Feb 21, 5:50 AM · debug-info
markus committed rG76dda218a06e: [DebugInfo] Prep llvm-dwarfdump for typed DW5 ops. (authored by markus).
[DebugInfo] Prep llvm-dwarfdump for typed DW5 ops.
Thu, Feb 21, 12:21 AM
markus committed rL354552: [DebugInfo] Prep llvm-dwarfdump for typed DW5 ops..
[DebugInfo] Prep llvm-dwarfdump for typed DW5 ops.
Thu, Feb 21, 12:20 AM
markus closed D58442: llvm-dwarfinfo add support to decode DW_OP_convert.
Thu, Feb 21, 12:20 AM · Restricted Project

Wed, Feb 20

markus updated the diff for D58442: llvm-dwarfinfo add support to decode DW_OP_convert.

Added support for rudimentary verify of expressions (basically to check that a BaseTypeRef operand actually points to a DW_TAG_base_type DIE).

Wed, Feb 20, 12:15 PM · Restricted Project
markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Wed, Feb 20, 11:21 AM · debug-info
markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.

+ Addressed review remark (use range-based loop instead of iterators).
+ Updated tests (probably forgot during last rebase. Verified the values used now with a clean master)

Wed, Feb 20, 7:05 AM · debug-info
markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

I have created a review for the requested 'llvm-dwarfdump' updates (the printing part) in https://reviews.llvm.org/D58442

Wed, Feb 20, 6:10 AM · debug-info
markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

I'm a touch confused by the whole discussion here - so I'll write some things & perhaps folks can correct my understanding where necessary.

I think that we all are. Maybe it is because we tried some many different approaches in the same review that it is a bit convoluted now.

The issue is that a location expression (either in a direct location, or in a loclist) needs to reference a DIE.

Correct.

Sounds like that DIE reference is necessarily CU-local (because we're talking about precomputing the offset - and we could only do that if it's CU-local).

Also correct. The spec says that the reference is an offset into the current CU.

We already emit other CU-local DIE references in attributes (eg: DW_AT_specification, etc) references with hardcoded 4 bytes in size- so why would it be problematic to emit this one in the same way (with a padded ULEB128 that we know will give us 4 bytes of offset to work with)?

Agree. It should be no more problematic than what is done in other places already. The 4 byte ULEB128 gives us 28 bits to encode the offset in but as reasoned in other comments there is no way that limit can be reached.

& yeah, maybe if someone wants to save us some size (at the cost of whatever computational complexity this invokes in the assembler) it could be replaced with label differences (doing label differences for all DIE references would be nifty for manual editing/mucking about with DWARF, but not a big deal).

Agree.

Wed, Feb 20, 6:07 AM · debug-info
markus updated the summary of D58442: llvm-dwarfinfo add support to decode DW_OP_convert.
Wed, Feb 20, 5:37 AM · Restricted Project
markus created D58442: llvm-dwarfinfo add support to decode DW_OP_convert.
Wed, Feb 20, 5:30 AM · Restricted Project

Tue, Feb 19

markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Tue, Feb 19, 10:25 AM · debug-info
markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Tue, Feb 19, 2:30 AM · debug-info
markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Tue, Feb 19, 1:38 AM · debug-info

Mon, Feb 18

markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.
  • Did some cleanup
  • Added tests
  • Emit DW_OP_convert for Dwarf5
  • Emit legacy shift and mask expression for Dwarf4 and lower
  • Added llc option -generate-typed-dwarf5-expr to force emission regardless of targeted Dwarf version
Mon, Feb 18, 2:35 AM · debug-info

Fri, Feb 15

markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.

Addressed the inlined location expressions.

Fri, Feb 15, 7:36 AM · debug-info

Wed, Feb 13

markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

NVPTX supports only DWARF2 and does not know anything about DWARF5 operations. Also, it does not support any type of the expression in the DWARF sections, except for <section_name>+-<int_offset>.

Wed, Feb 13, 7:11 AM · debug-info
markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

I see now that the inlined places i.e. these:

git grep addBlock.*DW_AT_location lib/CodeGen/AsmPrinter
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:    addBlock(*VariableDIE, dwarf::DW_AT_location, DwarfExpr->finalize());
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:        addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:  addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
lib/CodeGen/AsmPrinter/DwarfUnit.cpp:        addBlock(ParamDIE, dwarf::DW_AT_location, Loc);

work quite differently and that will need some additional work.

Wed, Feb 13, 6:40 AM · debug-info
markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Wed, Feb 13, 12:32 AM · debug-info

Tue, Feb 12

markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Tue, Feb 12, 11:50 AM · debug-info
markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.

Removed Label from DIE (added DenseMap to CU and pass it around). Rebased to top of trunk.

Tue, Feb 12, 2:47 AM · debug-info

Mon, Feb 11

markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Mon, Feb 11, 3:43 AM · debug-info
markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.

Addresses most of the open issues I had. The major question that remains is what to do when not targeting Dwarf v5.

Mon, Feb 11, 3:33 AM · debug-info

Thu, Feb 7

markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Thu, Feb 7, 6:33 AM · debug-info
markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.

Updated to DW_OP_LLVM_convert as suggested, did the pretty printing and updated a bunch of llvm-lit tests to cope with the newly generated labels. Now the IR format looks like this:

!DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)

There are some open issues that I would like feedback on. I will add inline comments at those locations.

Thu, Feb 7, 6:02 AM · debug-info

Wed, Feb 6

markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

All three of these operations expect a DW_TAG_basic_type as argument. So we could easily introduce a DW_OP_LLVM_basic_type <signedness> <bits> operation that gets expanded in AsmPrinter into DW_TAG_basic_types like you do now and thus avoid having to deal with TrackingMDRefs. This would also reduce the memory footprint of all DIExpressions that don't need a type argument.

Wed, Feb 6, 6:46 AM · debug-info
markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.

Dropping all the metadata stuff and specifying the bit size and type encoding directly in the expression vector. e.g.

!DIExpression(DW_OP_convert, 8, 5, DW_OP_convert, 32, 5, DW_OP_stack_value)

This approach certainly has its advantages and require far less changes to existing code.

Wed, Feb 6, 6:42 AM · debug-info
markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Wed, Feb 6, 1:07 AM · debug-info

Tue, Feb 5

markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Tue, Feb 5, 7:23 AM · debug-info
markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.
Dwarf5 DW_OP_convert support.
Tue, Feb 5, 7:17 AM · debug-info
markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

If you choose this path, one way to solve the issue of what to do with the dangling types that are produced by the conversion operations would be to create them in a separate DICompileUnit with a compiler-generated name and location. This way the types should get uniqued automatically during LTO, and you don't need to worry about llvm-link & friends.

Tue, Feb 5, 6:54 AM · debug-info

Mon, Feb 4

markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

the type reference should be a metadata operand. Otherwise you'll need to implement support in llvm-link etc, too.
I would just implement the type reference as normal metadata operands in DIExpression and bake the knowledge that DW_OP_convert consumes one of the metadata operands into the DIExpression operand iterator.

Mon, Feb 4, 5:44 AM · debug-info

Fri, Feb 1

markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.

Time to switch the approach. This time try adding support for typed Dwarf5 stack ops such as DW_OP_convert.

Fri, Feb 1, 6:37 AM · debug-info

Mon, Jan 28

markus added inline comments to D56587: Introduce DW_OP_LLVM_convert.
Mon, Jan 28, 7:39 AM · debug-info
markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.

Insert DW_OP_deref_size in Prologue Epilogue Inserter.

Mon, Jan 28, 7:33 AM · debug-info

Thu, Jan 24

markus added a comment to D57010: Fix sign/zero extension in Dwarf expressions (with pseudo ops).

Can you summarize what the advantage of the new operation as opposed to using DWARF 5 type conversions would be?

Thu, Jan 24, 6:50 AM · debug-info

Jan 21 2019

markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

With the fragment approach we still have problems in e.g. lib/CodeGen/PrologEpilogInserter.cpp:1105 where the stack location of a function argument is prepended to the expression and a sized DW_OP_deref would need to be inserted.

Jan 21 2019, 4:51 AM · debug-info
markus created D57010: Fix sign/zero extension in Dwarf expressions (with pseudo ops).
Jan 21 2019, 3:17 AM · debug-info

Jan 17 2019

markus updated the diff for D56587: Introduce DW_OP_LLVM_convert.

Uploading a prototype implementation using the DW_OP_LLVM_fragment as @bjope described. There are still some issues that need to be worked out with this so it is not final but rather I would like some input on if going down this track would be considered an acceptable solution. If there are clear objections to this approach it would be good to hear them early.

Jan 17 2019, 6:47 AM · debug-info

Jan 15 2019

markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

You wouldn't hardcode the offset in IR, you'd refer to the DIType as an MDNode reference and then teach the backend to resolve the reference, similar to how DIE references are resolved in the entire DIType class hierarchy.

In LLVM Assembly this could look like:

!1 = !DIBasicType(name: "short", ...)
!2 = !DIExpression(DW_OP_convert, !1)

but the actual implementation would be as I outlined in my previous reply.

Jan 15 2019, 4:02 AM · debug-info

Jan 14 2019

markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

Referencing a DIE in DIExpression would need a bit of additional work. DIExpression stores its operands in an array of uint64_t, so in order to support MDNode operands we'd have to add them as actual MDNode operands. For example, expr_op_iterator could know which DIExpression operations take MDNode arguments and inject them in the right places. Slightly more complicated would be actually finding a matching DIType in the debug info that we want to point to. If we want to convert to exactly the type of the DIVariable, we can use that type, otherwise we might have to generate new DIBasicTypes on the fly.

Even though this sounds complicated, I still think that it is preferable to encode type conversions as such rather than generating really complicated expressions that happen to have the same effect in the underspecified DWARF 4 stack language.

Jan 14 2019, 10:22 AM · debug-info
markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

Unfortunately I think that there are additional complications in that our sext/zext computations will have to also depend on the endianness of the target and if the value is stored in memory or not (as having the value loaded as the 'wrong type' would require bytes to be swapped on a big endian target before it could be sext/zext by this expression).

Jan 14 2019, 3:21 AM · debug-info
markus added a comment to D56587: Introduce DW_OP_LLVM_convert.

ii) It is probably not safe to assume that the consumer/debugger would set the high bits to zero in zero extension in e.g. the case that the variable has been spilled to memory.

Why? The consumer should have truncate the fragment to its DW_AT_size, right?

Jan 14 2019, 12:19 AM · debug-info

Jan 13 2019

markus added a comment to D56587: Introduce DW_OP_LLVM_convert.
In D56587#1354394, @vsk wrote:

Thanks for the patch.

t’s not obvious to me from skimming what the bug is with sign extension expressions. Could you describe what goes wrong, and maybe share a small program which shows the debugger behaving incorrectly?

Jan 13 2019, 11:27 PM · debug-info

Jan 11 2019

markus created D56587: Introduce DW_OP_LLVM_convert.
Jan 11 2019, 4:04 AM · debug-info

Jan 3 2019

markus committed rL350289: [CodeGen] Skip over dbg-instr in twoaddr pass.
[CodeGen] Skip over dbg-instr in twoaddr pass
Jan 3 2019, 12:40 AM
markus closed D55987: [CodeGen] Skip over dbg-instr in twoaddr pass.
Jan 3 2019, 12:39 AM

Jan 2 2019

markus updated the diff for D55107: Prototype BasicAA improvements for discussion on llvm-dev.
Jan 2 2019, 12:44 AM
markus added inline comments to D55987: [CodeGen] Skip over dbg-instr in twoaddr pass.
Jan 2 2019, 12:06 AM

Jan 1 2019

markus updated the diff for D55987: [CodeGen] Skip over dbg-instr in twoaddr pass.
Jan 1 2019, 11:57 PM

Dec 27 2018

markus added inline comments to D55987: [CodeGen] Skip over dbg-instr in twoaddr pass.
Dec 27 2018, 12:04 AM

Dec 21 2018

markus created D55987: [CodeGen] Skip over dbg-instr in twoaddr pass.
Dec 21 2018, 1:08 AM

Dec 7 2018

markus committed rL348570: [PM] Port LoadStoreVectorizer to the new pass manager..
[PM] Port LoadStoreVectorizer to the new pass manager.
Dec 7 2018, 12:27 AM
markus closed D54848: [PM] Port LoadStoreVectorizer to the new pass manager..
Dec 7 2018, 12:27 AM

Dec 6 2018

markus committed rL348483: Test commit: Removed trailing space in .txt file..
Test commit: Removed trailing space in .txt file.
Dec 6 2018, 5:23 AM

Dec 5 2018

markus updated the diff for D55107: Prototype BasicAA improvements for discussion on llvm-dev.

Did some more experiments, trying to clean up the code and make it more generic.
Instead of only determining if X != 0 (which is clearly not sufficient) try to produce a value range for the variable and use that.

Dec 5 2018, 1:57 AM

Nov 30 2018

markus added a comment to D54848: [PM] Port LoadStoreVectorizer to the new pass manager..

Thanks for reviewing :)

Nov 30 2018, 5:38 AM
markus added a comment to D55107: Prototype BasicAA improvements for discussion on llvm-dev.

I think we need to check whether shl/mul has nsw flag.

For example, 'shl i16 1024, 10' and 'shl i16 1024, 11' are both 0, so we need to check whether they don't overflow.

Nov 30 2018, 3:01 AM
markus created D55107: Prototype BasicAA improvements for discussion on llvm-dev.
Nov 30 2018, 1:55 AM

Nov 23 2018

markus added inline comments to D54848: [PM] Port LoadStoreVectorizer to the new pass manager..
Nov 23 2018, 2:59 AM
markus created D54848: [PM] Port LoadStoreVectorizer to the new pass manager..
Nov 23 2018, 2:53 AM

Nov 21 2018

markus updated the diff for D54695: [PM] Port Scalarizer to the new pass manager..
Nov 21 2018, 3:31 AM
markus added a comment to D54695: [PM] Port Scalarizer to the new pass manager..

I do not have a strong opinion about renaming 'transform' to 'visit' and would be happy to comply but simply doing a rename does not compile giving errors such as

Nov 21 2018, 1:15 AM

Nov 20 2018

markus updated the diff for D54695: [PM] Port Scalarizer to the new pass manager..

Moved ParallelLoopAccessMDKind into constructor.

Nov 20 2018, 6:20 AM
markus updated the diff for D54695: [PM] Port Scalarizer to the new pass manager..

Hopefully this patch solves most of the comments I have received so far.

Nov 20 2018, 5:23 AM

Nov 19 2018

markus updated the diff for D54695: [PM] Port Scalarizer to the new pass manager..

Split into one wrapper class for the legacy PM and one wrapper class for the new PM.

Nov 19 2018, 11:38 PM
markus added a comment to D54695: [PM] Port Scalarizer to the new pass manager..

Agree that splitting would give a cleaner solution. Will give that a go.

Nov 19 2018, 5:17 AM
markus edited reviewers for D54695: [PM] Port Scalarizer to the new pass manager., added: chandlerc, fedor.sergeev; removed: llvm-commits, rsandifo.
Nov 19 2018, 4:18 AM
markus created D54695: [PM] Port Scalarizer to the new pass manager..
Nov 19 2018, 3:48 AM