HsiangKai (Hsiangkai Wang)
User

Projects

User does not belong to any projects.

User Details

User Since
May 4 2016, 7:01 PM (115 w, 2 d)

Recent Activity

Sun, Jul 8

HsiangKai added inline comments to D45556: [DebugInfo] Generate DWARF debug information for labels..
Sun, Jul 8, 11:15 PM
HsiangKai updated the diff for D45556: [DebugInfo] Generate DWARF debug information for labels..
Sun, Jul 8, 11:09 PM

Thu, Jul 5

HsiangKai added inline comments to D45556: [DebugInfo] Generate DWARF debug information for labels..
Thu, Jul 5, 6:33 PM
HsiangKai updated the diff for D45556: [DebugInfo] Generate DWARF debug information for labels..
Thu, Jul 5, 6:27 PM

Tue, Jul 3

HsiangKai added a comment to D45045: [DebugInfo] Generate debug information for labels..

This broke the Chromium build. I've uploaded a reproducer at https://bugs.chromium.org/p/chromium/issues/detail?id=841170#c1

I'm guessing maybe a Clang bootstrap with debug info might also reproduce the problem, but I haven't tried that.

Reverted in r331861.

https://reviews.llvm.org/D46738 should fix the bug.

Tue, Jul 3, 1:07 AM

Mon, Jul 2

HsiangKai added a comment to D45556: [DebugInfo] Generate DWARF debug information for labels..

Ping.

Mon, Jul 2, 12:02 AM
HsiangKai added a comment to D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line..

Ping.

Mon, Jul 2, 12:02 AM

Jun 7 2018

HsiangKai added a comment to D45556: [DebugInfo] Generate DWARF debug information for labels..

This is not super important, but: contrary to to DBG_VALUE intrinsics, DBG_LABELs don't have live ranges associated with them. I wonder if we really need to rename DbgValueHistoryCalculator. We're not calculating any "history" of labels, do we?

Jun 7 2018, 11:27 PM
HsiangKai added a comment to D45556: [DebugInfo] Generate DWARF debug information for labels..

Thank you for the patch.

It seems to me that with requireLabelBeforeInsn, the label will later be emitted as a temporary symbol with a unique but unspecified name (in DebugHandleBase::beginInstruction()). Is it correct?

Yes, I generate a temporary label before the label intrinsic. In this way, I could get the address of the label afterward.

Yes, I mean that the label will later be named differently in the asm file than in the source? I don't know if this is something we should care about though, I'm just speaking from an user point of view

Jun 7 2018, 12:00 AM

Jun 6 2018

HsiangKai updated the diff for D46738: [DebugInfo] Fix PR37395..
Jun 6 2018, 11:31 PM
HsiangKai added a comment to D46738: [DebugInfo] Fix PR37395..

If that is the case then you should be able to write the assertion as

assert((isa<DILabelInst>() || !cast<MDNode>(MD)->getNumOperands()) && ...);

without guarding it in if (AllowNullOp). I'd be more comfortable this way since it is more explicit in that dbg.value's may not have null arguments. My point is that a dbg.value with a null argument is indicative of a compiler bug. We need to relax the assertion in order to support dbg.label (that's fine) but while doing that we shouldn't also relax it for dbg.value. Please let me know if I'm misunderstanding something.

Jun 6 2018, 11:14 PM
HsiangKai added a comment to D46738: [DebugInfo] Fix PR37395..

My general problem with this is that a debug info intrinsic with a null operand is usually indicative of a bug in a transformation pass that forgot to update the debug intrinsic after modifying the value it was pointing to. If the value goes away then the debug intrinsic should use an undef value (to end the range started by an earlier dbg intrinsic describing the same variable) or be removed entirely (if all instructions dealing with that variable have been removed as dead code). I don't want to make it easier to for transformation passes to not update the debug info correctly. If all you need is that DILabels are handled correctly, then this patch should just specifically enable that case.

Debug info intrinsic with a null operand could not indicate a bug in transformation passes, in getVariableLocation() at least. It will return nullptr in getVariableLocation() and the caller just skip the intrinsic.

Following is the code fragment from llvm::insertDebugValuesForPHIs()

ValueToValueMapTy DbgValueMap;
for (auto &I : *BB) {
  if (auto DbgII = dyn_cast<DbgInfoIntrinsic>(&I)) {
    if (auto *Loc = dyn_cast_or_null<PHINode>(DbgII->getVariableLocation()))
      DbgValueMap.insert({Loc, DbgII});
  }
}

You could see that when getVariableLocation() return nullptr, it just skips the intrinsic.

The assertion is triggered when the intrinsic has operands and the first operand is not value, not has a null operand.

Do you think you could post an example were this assertion incorrectly fires for a dbg,value intrinsic? I would really like to understand this case better.

Jun 6 2018, 8:06 PM

May 31 2018

HsiangKai added a comment to D46738: [DebugInfo] Fix PR37395..

My general problem with this is that a debug info intrinsic with a null operand is usually indicative of a bug in a transformation pass that forgot to update the debug intrinsic after modifying the value it was pointing to. If the value goes away then the debug intrinsic should use an undef value (to end the range started by an earlier dbg intrinsic describing the same variable) or be removed entirely (if all instructions dealing with that variable have been removed as dead code). I don't want to make it easier to for transformation passes to not update the debug info correctly. If all you need is that DILabels are handled correctly, then this patch should just specifically enable that case.

May 31 2018, 12:28 AM
HsiangKai added a comment to D45556: [DebugInfo] Generate DWARF debug information for labels..

Thank you for the patch.

It seems to me that with requireLabelBeforeInsn, the label will later be emitted as a temporary symbol with a unique but unspecified name (in DebugHandleBase::beginInstruction()). Is it correct?

Yes, I generate a temporary label before the label intrinsic. In this way, I could get the address of the label afterward.

May 31 2018, 12:05 AM

May 28 2018

HsiangKai added a comment to D46738: [DebugInfo] Fix PR37395..

The fix is also related to broken Chromium build due to D45045.
https://bugs.chromium.org/p/chromium/issues/detail?id=841170#c1

May 28 2018, 10:25 PM
HsiangKai added a comment to D45045: [DebugInfo] Generate debug information for labels..

This broke the Chromium build. I've uploaded a reproducer at https://bugs.chromium.org/p/chromium/issues/detail?id=841170#c1

I'm guessing maybe a Clang bootstrap with debug info might also reproduce the problem, but I haven't tried that.

Reverted in r331861.

May 28 2018, 10:23 PM

May 27 2018

HsiangKai added a comment to D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line..

It looks to me like the line-table emitted by the new path would be valid. But I am not comfortable reviewing things related to MCExprs and fragments.

May 27 2018, 11:15 PM
HsiangKai added a comment to D46738: [DebugInfo] Fix PR37395..

getVariableLocation() assumes all DbgInfoIntrinsic has the address value as its first operand. It is not the case for DILabel.

When the assertion is triggered, MDNode is DILabel. The first operand of DILabel is a metadata to keep label information, not address value. So, the second if contition, dyn_cast<ValueAsMetadata>, will return null.

It adds an assertion to ensure the number of operand must be zero before returning nullptr. I am not sure it is reasonable to check the condition before return. Is there any case it must ensure the number of operands be zero before returning nullptr?

Why aren't you checking whether this is a DILabel instead?

Although the fix is for DILabel now, I think it is a more general fix for DbgInfoIntrinsic without address as its first operand.

May 27 2018, 10:38 PM

May 24 2018

HsiangKai added a comment to D41572: [MachineTraceMetrics] Fix bug in pickTracePred.

Ping.

May 24 2018, 11:07 PM
HsiangKai added a comment to D45556: [DebugInfo] Generate DWARF debug information for labels..

Refactoring is done.

May 24 2018, 11:04 PM
HsiangKai updated the diff for D45556: [DebugInfo] Generate DWARF debug information for labels..
May 24 2018, 11:00 PM
HsiangKai added a comment to D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line..

Create a simple slide to describe the problem in current LLVM implementation.
If there is no relocation in .debug_line, the program will be undebuggable after relaxation.

May 24 2018, 10:55 PM
HsiangKai updated the diff for D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line..
May 24 2018, 12:32 AM

May 21 2018

HsiangKai added a comment to D46738: [DebugInfo] Fix PR37395..

Update the test case according to comments is done.

May 21 2018, 7:00 PM
HsiangKai added a comment to D46738: [DebugInfo] Fix PR37395..

getVariableLocation() assumes all DbgInfoIntrinsic has the address value as its first operand. It is not the case for DILabel.

When the assertion is triggered, MDNode is DILabel. The first operand of DILabel is a metadata to keep label information, not address value. So, the second if contition, dyn_cast<ValueAsMetadata>, will return null.

It adds an assertion to ensure the number of operand must be zero before returning nullptr. I am not sure it is reasonable to check the condition before return. Is there any case it must ensure the number of operands be zero before returning nullptr?

Why aren't you checking whether this is a DILabel instead?

May 21 2018, 6:59 PM
HsiangKai updated the diff for D46738: [DebugInfo] Fix PR37395..

Update test case.

May 21 2018, 3:23 AM

May 20 2018

HsiangKai updated the diff for D46738: [DebugInfo] Fix PR37395..

Update test case.

May 20 2018, 8:11 PM
HsiangKai added inline comments to D46738: [DebugInfo] Fix PR37395..
May 20 2018, 7:15 PM
HsiangKai updated the diff for D46738: [DebugInfo] Fix PR37395..

If getVariableLocation() allows to return nullptr and the first operand
of the intrinsic function is not address value, we just return nullptr
without checking the number of operands be zero.

May 20 2018, 7:14 PM
HsiangKai added a comment to D46738: [DebugInfo] Fix PR37395..

Neither the PR nor this review contain any information on why removing the assertion is the correct action here. Can you explain why this is the right fix, and what the MDNode is when the assertion fires?

May 20 2018, 7:01 PM

May 16 2018

HsiangKai added a reviewer for D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line.: aprantl.
May 16 2018, 12:07 AM
HsiangKai added reviewers for D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line.: asb, edward-jones, simoncook.
May 16 2018, 12:03 AM

May 14 2018

HsiangKai added a dependent revision for D45181: [RISCV] Add diff relocation support for RISC-V: D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line..
May 14 2018, 2:17 PM
HsiangKai added a dependent revision for D45773: [RISCV] Don't fold symbol diff: D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line..
May 14 2018, 2:17 PM
HsiangKai added dependencies for D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line.: D45181: [RISCV] Add diff relocation support for RISC-V, D45773: [RISCV] Don't fold symbol diff.
May 14 2018, 2:17 PM
HsiangKai created D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line..
May 14 2018, 2:13 PM

May 13 2018

HsiangKai added a comment to D46738: [DebugInfo] Fix PR37395..

The fix itself looks fine, but I'll defer to @aprantl.
Do you need anybody to commit this for you?

May 13 2018, 10:19 PM

May 10 2018

HsiangKai added a comment to D45342: [DebugInfo] Examine all uses of isDebugValue() for debug instructions..

Create a new revision to review it.
https://reviews.llvm.org/D46739

May 10 2018, 8:09 PM
HsiangKai created D46739: [DebugInfo] Only handle DBG_VALUE in InlineSpiller.
May 10 2018, 8:06 PM
HsiangKai created D46738: [DebugInfo] Fix PR37395..
May 10 2018, 8:01 PM

May 9 2018

HsiangKai added inline comments to D45342: [DebugInfo] Examine all uses of isDebugValue() for debug instructions..
May 9 2018, 8:08 PM

May 8 2018

HsiangKai added reviewers for D41572: [MachineTraceMetrics] Fix bug in pickTracePred: spatel, thegameg, dblaikie.
May 8 2018, 7:40 PM

Apr 26 2018

HsiangKai added a comment to D45556: [DebugInfo] Generate DWARF debug information for labels..

TODO: Refactor the patch to abstract the processing flow of abstract variables and abstract labels. The refactoring will start after mid of May.

Apr 26 2018, 11:28 PM
HsiangKai added a comment to D45045: [DebugInfo] Generate debug information for labels..

@chenwj Do you have any comments for this patch?

Apr 26 2018, 11:23 PM
HsiangKai added inline comments to D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..
Apr 26 2018, 11:10 PM
HsiangKai updated the diff for D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..

Add test case.

Apr 26 2018, 11:09 PM
HsiangKai updated the diff for D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label..
  • Update according to review comments.
Apr 26 2018, 11:07 PM

Apr 20 2018

HsiangKai added inline comments to D45556: [DebugInfo] Generate DWARF debug information for labels..
Apr 20 2018, 1:13 AM
HsiangKai updated the diff for D45045: [DebugInfo] Generate debug information for labels..
  • Update test cases.
  • Checked with clang-format.
Apr 20 2018, 12:40 AM
HsiangKai updated the diff for D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..
  • Update the test case.
Apr 20 2018, 12:36 AM
HsiangKai updated the diff for D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label..
  • Rename elements in DISubprogram to retainedNodes.
  • Update test cases.
  • Update according to review comments.
Apr 20 2018, 12:32 AM

Apr 17 2018

HsiangKai added a comment to D45556: [DebugInfo] Generate DWARF debug information for labels..

About combining DwarfLabel and DbgVariable.

Apr 17 2018, 12:23 AM

Apr 16 2018

HsiangKai added a comment to D45045: [DebugInfo] Generate debug information for labels..

Always add labels to DISubprogram even unreachable.

Apr 16 2018, 8:00 AM
HsiangKai added inline comments to D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..
Apr 16 2018, 7:47 AM
HsiangKai updated the diff for D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..
  1. Use range-based for in lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:918
  2. Check !dbg attachment in the test case.
Apr 16 2018, 7:46 AM
HsiangKai added inline comments to D45556: [DebugInfo] Generate DWARF debug information for labels..
Apr 16 2018, 7:40 AM
HsiangKai added a comment to D45395: [RISCV] Lower the tail pseudoinstruction.

Two questions.

Apr 16 2018, 12:31 AM
HsiangKai requested changes to D45395: [RISCV] Lower the tail pseudoinstruction.

I think it should be a typo.

Apr 16 2018, 12:01 AM

Apr 14 2018

HsiangKai updated the diff for D45045: [DebugInfo] Generate debug information for labels..

Update test cases.

Apr 14 2018, 7:02 PM
HsiangKai updated the diff for D45556: [DebugInfo] Generate DWARF debug information for labels..

Update according to reviewers' comments.
Update test cases.

Apr 14 2018, 7:00 PM
HsiangKai updated the diff for D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..

Update according to reviewers' comments.

Apr 14 2018, 6:59 PM
HsiangKai added a comment to D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label..

I realize it's non-trivial to keep these all in the same list - but I'd
rather not create a new list only to remove it again later if it can be
helped. What particular issues did you encounter when attempting to use a
single list?

Apr 14 2018, 6:56 PM
HsiangKai updated the diff for D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label..

Merge label metadata list into variable metadata list. The new metadata list is called 'elements' list. I do not merge other metadata lists into this one.

Apr 14 2018, 6:55 PM

Apr 12 2018

HsiangKai added a reviewer for D45045: [DebugInfo] Generate debug information for labels.: aprantl.
Apr 12 2018, 1:20 AM
HsiangKai abandoned D45043: [DebugInfo] Add test cases for generating debug info of labels..

Merged into D45556.

Apr 12 2018, 1:17 AM
HsiangKai added a dependency for D45556: [DebugInfo] Generate DWARF debug information for labels.: D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..
Apr 12 2018, 1:15 AM
HsiangKai added a dependent revision for D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr.: D45556: [DebugInfo] Generate DWARF debug information for labels..
Apr 12 2018, 1:15 AM
HsiangKai created D45556: [DebugInfo] Generate DWARF debug information for labels..
Apr 12 2018, 1:15 AM

Apr 11 2018

HsiangKai updated the diff for D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label..

Add DILabel list to DISubprogram. The new DISubprogram will be

Apr 11 2018, 7:17 AM
HsiangKai updated the diff for D45342: [DebugInfo] Examine all uses of isDebugValue() for debug instructions..

Update according to review comments.

Apr 11 2018, 7:04 AM
HsiangKai added a comment to D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..

Thanks, this looks mostly fine! Have you considered just allowing the DBG_VALUE MachineInstr to carry a DILabel insetad of just DILocalVariables? Would that simplify the code or make it harder to understand?

Apr 11 2018, 7:04 AM
HsiangKai updated the diff for D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..

Update according to review comments.

Apr 11 2018, 6:16 AM
HsiangKai updated the diff for D45045: [DebugInfo] Generate debug information for labels..

Update the test case for inlined functions. The inlined label will attach to DISubprogram(..., labels: !n)

Apr 11 2018, 6:12 AM

Apr 5 2018

HsiangKai added a comment to D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label..

Probably best not to create another list on the DISubprogram - you might be
able to generalize the existing list to contain "anything that might be
optimized out"

Katya (CC'd), over at Sony, has been looking at a similar issue/possible
change to move function-local using declarations/directives into such a
list as well.

Apr 5 2018, 5:41 PM
HsiangKai added a comment to D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label..

My understanding was that in the latest revision of the design we don't have anywhere to attach the DILabel to when it is optimized away. If we do care about DILabels that have been optimized away (I don't see why we would want to) we would have to add a list of labels to the DISubprogram similar to how we keep dead variables around. But I don't htink that that is worth it.

Hmm if I compile a function containing 3 labels and the debugger knows about only 2, I might think the compiler has a bug.
We keep dead variables around so we can correctly report they have been optimized away. Thinking ahead to supporting languages where labels are far more common than in C/C++ (for example I know the OpenVMS guys will want to support COBOL) I think a labels list might be appropriate.

In the end I'm fine with both variants, but if we go for the line field in the DILabel I think we should also implement support for the labels list in DISubprogram or at least document our intention to add this clearly.

Apr 5 2018, 5:32 PM
HsiangKai added a dependency for D45342: [DebugInfo] Examine all uses of isDebugValue() for debug instructions.: D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..
Apr 5 2018, 5:17 PM
HsiangKai added a dependent revision for D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr.: D45342: [DebugInfo] Examine all uses of isDebugValue() for debug instructions..
Apr 5 2018, 5:17 PM
HsiangKai added a dependency for D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr.: D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label..
Apr 5 2018, 5:17 PM
HsiangKai added a dependent revision for D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label.: D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..
Apr 5 2018, 5:17 PM
HsiangKai created D45342: [DebugInfo] Examine all uses of isDebugValue() for debug instructions..
Apr 5 2018, 5:13 PM
HsiangKai created D45341: [DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr..
Apr 5 2018, 5:10 PM

Apr 2 2018

HsiangKai added inline comments to D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label..
Apr 2 2018, 10:41 PM
HsiangKai added a comment to D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label..

D45045 is the clang part of the implementation to support DILabel and llvm.dbg.label.

Apr 2 2018, 8:20 AM
HsiangKai added a dependent revision for D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label.: D45045: [DebugInfo] Generate debug information for labels..
Apr 2 2018, 8:15 AM
HsiangKai removed a dependent revision for D45043: [DebugInfo] Add test cases for generating debug info of labels.: D45045: [DebugInfo] Generate debug information for labels..
Apr 2 2018, 8:15 AM
HsiangKai edited dependencies for D45045: [DebugInfo] Generate debug information for labels., added: 1; removed: 1.
Apr 2 2018, 8:15 AM
HsiangKai updated the diff for D45045: [DebugInfo] Generate debug information for labels..
Apr 2 2018, 8:15 AM
HsiangKai abandoned D45025: [DebugInfo] Add parser for DILabel metadata..

Needs a test. Probably should be combined with the Verifier patch as well.

Apr 2 2018, 8:14 AM
HsiangKai abandoned D45089: [DebugInfo] Verifier for DILabel..

This wants to have a test, which probably means it should be combined with one or more of the other patches that support creating/parsing the label metadata.

Apr 2 2018, 8:11 AM
HsiangKai abandoned D45088: [DebugInfo] Prepare DIBuilder for labels.

I suggest combining this with a patch that actually uses the API. We don't generally have commits that add APIs without uses or tests.

Apr 2 2018, 8:10 AM
HsiangKai abandoned D45078: [DebugInfo] Enable the capability of attaching metadata to BasicBlock..

As discussed in llvm-dev mailing list, we should attach label metadata in intrinsic instead of basic block.

Apr 2 2018, 8:09 AM
HsiangKai abandoned D45032: [DebugInfo] LLVM IR assembly writer for DILabel..

LGTM. However, combining this one with D45025 would be better.

Apr 2 2018, 8:07 AM
HsiangKai abandoned D45026: [DebugInfo] Add bitcode reader/writer for DILabel metadata..

It's usual to do bitcode and text reading/writing for new metadata all in one patch; splitting it up actually makes it harder to review.

Apr 2 2018, 8:05 AM
HsiangKai updated the diff for D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label..

Restructure patches and add more test cases.

Apr 2 2018, 8:00 AM

Mar 30 2018

HsiangKai updated the diff for D45045: [DebugInfo] Generate debug information for labels..
  1. No generating llvm.dbg.label intrinsic.
  2. Modify commits according to chenwj's comments.
Mar 30 2018, 7:21 AM
HsiangKai created D45089: [DebugInfo] Verifier for DILabel..
Mar 30 2018, 6:50 AM
HsiangKai created D45088: [DebugInfo] Prepare DIBuilder for labels.
Mar 30 2018, 6:49 AM
HsiangKai abandoned D45042: [DebugInfo] Fill DW_TAG_label attributes..
Mar 30 2018, 1:48 AM
HsiangKai abandoned D45041: [DebugInfo] Construct DW_TAG_label DIEs from DbgLabels..
Mar 30 2018, 1:45 AM
HsiangKai abandoned D45040: [DebugInfo] Construct DWARF-specific data for labels in DwarfDebug..
Mar 30 2018, 1:44 AM