Page MenuHomePhabricator

bjope (Bjorn Pettersson)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 26 2016, 7:58 AM (134 w, 2 d)

Recent Activity

Today

bjope added a comment to D60831: [DebugInfo@O2][LoopVectorize] pr39024: Vectorized code linenos step through loop even after completion.

This will be my last comment on the topic and then everyone can get back to work :-). I agree with the LLVM IR requirement that one not to lie to application developers; lies are worse than nothing. However, I don't feel that you have adequately explained why attributing to any of the available (file, line) mappings for an instruction that has been merged is incorrect and not 100% reliable. Just because we can't include all contributing mappings for a merged instruction doesn't make any one unreliable; I see attributing to any one of a set of mappings as 100% correct, even though it is only partial information.

Sorry for jumping in after you said you would stop, but I have been away.
Personally I find this example compelling: When there's an if/then/else construct, and some instruction is hoisted above the if, you could assign it to (for example) the source location of the 'then' block. Now in your training run, the 'then' block can be given 100% of executions (because that's what the hoisted instruction says) even if the 'else' block was chosen 100% of the time. I find the resulting profile is "incorrect and not 100% reliable" and when used for PGO will produce sub-optimal code. It is hard to imagine any other valid conclusion for this use-case.

As Adrian mentioned, being able to attribute multiple locations would be preferable to attributing line 0, and I hope we can make that happen in the future.

Wed, Apr 24, 7:38 AM · debug-info, Restricted Project

Yesterday

bjope committed rG71e8c6f20fe4: Add "const" in GetUnderlyingObjects. NFC (authored by bjope).
Add "const" in GetUnderlyingObjects. NFC
Tue, Apr 23, 11:57 PM
bjope committed rL359072: Add "const" in GetUnderlyingObjects. NFC.
Add "const" in GetUnderlyingObjects. NFC
Tue, Apr 23, 11:57 PM
bjope closed D61038: Add "const" in GetUnderlyingObjects.
Tue, Apr 23, 11:57 PM · Restricted Project
bjope added a comment to D61038: Add "const" in GetUnderlyingObjects.

Seems like a NFC.
Looks awesome to me - const correctness for the win!

Tue, Apr 23, 11:10 PM · Restricted Project
bjope created D61038: Add "const" in GetUnderlyingObjects.
Tue, Apr 23, 2:20 PM · Restricted Project
bjope added inline comments to D58704: Initial (incomplete) implementation of JITLink - A replacement for RuntimeDyld..
Tue, Apr 23, 1:52 PM · Restricted Project
bjope added inline comments to D60715: [ISEL][X86] Tracking of registers that forward call arguments.
Tue, Apr 23, 1:45 PM · debug-info
bjope added inline comments to D60715: [ISEL][X86] Tracking of registers that forward call arguments.
Tue, Apr 23, 1:37 PM · debug-info
bjope committed rGf97b29be884c: [DAGCombiner] Combine OR as ADD when no common bits are set (authored by bjope).
[DAGCombiner] Combine OR as ADD when no common bits are set
Tue, Apr 23, 3:02 AM
bjope committed rL358965: [DAGCombiner] Combine OR as ADD when no common bits are set.
[DAGCombiner] Combine OR as ADD when no common bits are set
Tue, Apr 23, 2:59 AM
bjope closed D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.
Tue, Apr 23, 2:59 AM · Restricted Project
bjope added inline comments to D53701: [Analyzer] Instead of recording comparisons in interator checkers do an eager state split.
Tue, Apr 23, 2:19 AM · Restricted Project

Fri, Apr 19

bjope added a comment to D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.

LGTM (see inline for a couple of nits) - but I'd prefer that someone with AMDGPU knowledge (@arsenm @nhaehnle @rampitec ?) confirm those diffs too.

Fri, Apr 19, 3:09 AM · Restricted Project
bjope committed rG18b0442560cc: [LibTooling] Fix -Wsign-compare after r358697 (authored by bjope).
[LibTooling] Fix -Wsign-compare after r358697
Fri, Apr 19, 2:11 AM
bjope committed rC358745: [LibTooling] Fix -Wsign-compare after r358697.
[LibTooling] Fix -Wsign-compare after r358697
Fri, Apr 19, 2:11 AM
bjope committed rL358745: [LibTooling] Fix -Wsign-compare after r358697.
[LibTooling] Fix -Wsign-compare after r358697
Fri, Apr 19, 2:11 AM
bjope committed rG238c9d6308df: [CodeGen] Add "const" to MachineInstr::mayAlias (authored by bjope).
[CodeGen] Add "const" to MachineInstr::mayAlias
Fri, Apr 19, 2:07 AM
bjope committed rL358744: [CodeGen] Add "const" to MachineInstr::mayAlias.
[CodeGen] Add "const" to MachineInstr::mayAlias
Fri, Apr 19, 2:07 AM
bjope closed D60856: [CodeGen] Add "const" to MachineInstr::mayAlias.
Fri, Apr 19, 2:06 AM · Restricted Project

Thu, Apr 18

bjope created D60856: [CodeGen] Add "const" to MachineInstr::mayAlias.
Thu, Apr 18, 1:17 AM · Restricted Project

Wed, Apr 17

bjope added a comment to D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.

If there still are worries about this, then maybe I can do the visitADD refactoring in a separate patch? And then I can limit this patch to the part where we start to use visitADDLike from visitOR.

Wed, Apr 17, 11:12 AM · Restricted Project
bjope added a comment to D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.

ping!

Wed, Apr 17, 11:00 AM · Restricted Project
bjope added a comment to D60715: [ISEL][X86] Tracking of registers that forward call arguments.

Looks generally good. But...

Wed, Apr 17, 12:18 AM · debug-info

Mon, Apr 15

bjope added inline comments to D60311: MIR printer should lowercase sub-register names to be in sync with parser?.
Mon, Apr 15, 2:10 AM · Restricted Project
bjope added inline comments to D60311: MIR printer should lowercase sub-register names to be in sync with parser?.
Mon, Apr 15, 2:09 AM · Restricted Project
bjope committed rG60569363a589: [SelectionDAG] Use KnownBits::computeForAddSub/computeForAddCarry (authored by bjope).
[SelectionDAG] Use KnownBits::computeForAddSub/computeForAddCarry
Mon, Apr 15, 12:19 AM
bjope committed rL358372: [SelectionDAG] Use KnownBits::computeForAddSub/computeForAddCarry.
[SelectionDAG] Use KnownBits::computeForAddSub/computeForAddCarry
Mon, Apr 15, 12:19 AM
bjope closed D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.
Mon, Apr 15, 12:19 AM · Restricted Project

Fri, Apr 12

bjope updated the diff for D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.

Rebased. Corrected typos in the test cases.

Fri, Apr 12, 2:09 PM · Restricted Project
bjope added inline comments to D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.
Fri, Apr 12, 8:32 AM · Restricted Project
bjope added inline comments to D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.
Fri, Apr 12, 8:32 AM · Restricted Project
bjope added a comment to D60522: [KnownBits] Add computeForAddCarry().

LGTM! (with a nit in the test case)

Fri, Apr 12, 4:56 AM · Restricted Project

Thu, Apr 11

bjope updated the diff for D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.

Updated to use a KnownBits::computeForAddCarry helper. I added a simple
implementation here, but the idea is to replace it by the improved version
from D60522 instead (if we land that patch before this one).

Thu, Apr 11, 8:47 AM · Restricted Project
bjope added inline comments to D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.
Thu, Apr 11, 2:19 AM · Restricted Project
bjope added inline comments to D60522: [KnownBits] Add computeForAddCarry().
Thu, Apr 11, 2:02 AM · Restricted Project
bjope updated the diff for D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.

Removed the add_zext_ifpos_vec_splat2 test from test/CodeGen/X86/signbit-shift.ll again (as suggested by @lebedev.ri). That test was only added to demonstrate why add_zext_ifpos_vec_splat gets an extra movdqa with this patch (due to unfortunate reg constraints), but it did not contribute anything new when it comes to testing "signbit-shift".

Thu, Apr 11, 1:28 AM · Restricted Project
bjope updated subscribers of D59648: [BasicAliasAnalysis] Fix computation for arrays > half of address space.
Thu, Apr 11, 12:22 AM · Restricted Project

Wed, Apr 10

bjope updated the diff for D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.

Simplified based on feedback.

Wed, Apr 10, 5:21 AM · Restricted Project
bjope added inline comments to D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.
Wed, Apr 10, 4:48 AM · Restricted Project
bjope retitled D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits from [SelectionDAG] Let computeKnownBits handle OR-like ADDs better to [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.
Wed, Apr 10, 2:39 AM · Restricted Project
bjope updated the diff for D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.

Use KnownBits::computeForAddSub instead, and do it for both ADD* and SUB* nodes.

Wed, Apr 10, 2:38 AM · Restricted Project
bjope added a comment to D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.

We have an accurate known bits implementation for adds in KnownBits::computeForAddSub(), which should handle this and more automatically. We use it to compute add known bits in IR. Is there a reason why it's not used for SDAG known bits?

Hmm, good point. In general because of IR Value vs DAG SDValue. But clearly that function should work here..

Wed, Apr 10, 1:21 AM · Restricted Project

Tue, Apr 9

bjope updated the diff for D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.

Added a unittest (by using the AArch64SelectionDAGTest unittest framework).

Tue, Apr 9, 1:07 PM · Restricted Project
bjope added a comment to D59626: [AMDGPU] Add MachineDCE pass after RenameIndependentSubregs.

For the record, I think the "multiple connected components" problem could have been solved by something like this after the call to "createAndComputeVirtRegInterval"

LiveInterval &LI = LIS->getInterval(Reg);
assert(Reg == LI.reg && "Invalid reg to interval mapping");
if (TargetRegisterInfo::isVirtualRegister(LI.reg)) {
  // Check whether or not LI is composed by multiple connected
  // components and if that is the case, fix that.
  SmallVector<LiveInterval*, 8> SplitLIs;
  LIS->splitSeparateComponents(LI, SplitLIs);
}

However, that still leaves the questions about if the pass should do less optimizations related to physregs when LIS is available, or how to preserve LIS if optimizing on physregs.
And I'm not sure about the cost of using splitSeparateComponents compared to a full recompute.

Tue, Apr 9, 9:16 AM · Restricted Project
bjope accepted D60466: Revert LIS handling in MachineDCE.

nit: Maybe you want to add the reproducer for the "multiple connected intervals" for future reference?

Tue, Apr 9, 9:02 AM · Restricted Project
bjope added a comment to D59626: [AMDGPU] Add MachineDCE pass after RenameIndependentSubregs.

@bjope could you please check if D60419 solves your problem with physreg use removal?

Tue, Apr 9, 8:30 AM · Restricted Project
bjope added inline comments to D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.
Tue, Apr 9, 8:00 AM · Restricted Project
bjope added a comment to D59626: [AMDGPU] Add MachineDCE pass after RenameIndependentSubregs.

Here is a MIR-reproducer triggering "Multiple connected components in live interval".

Tue, Apr 9, 7:28 AM · Restricted Project
bjope added a comment to D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.

Friendly ping!

Tue, Apr 9, 6:29 AM · Restricted Project
bjope created D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits.
Tue, Apr 9, 6:29 AM · Restricted Project
bjope committed rC357985: [ASTImporter] Fix in ASTImporter::Import_New(const Decl *).
[ASTImporter] Fix in ASTImporter::Import_New(const Decl *)
Tue, Apr 9, 2:11 AM
bjope committed rG5cca2c25a7dd: [ASTImporter] Fix in ASTImporter::Import_New(const Decl *) (authored by bjope).
[ASTImporter] Fix in ASTImporter::Import_New(const Decl *)
Tue, Apr 9, 2:11 AM
bjope committed rL357985: [ASTImporter] Fix in ASTImporter::Import_New(const Decl *).
[ASTImporter] Fix in ASTImporter::Import_New(const Decl *)
Tue, Apr 9, 2:11 AM

Mon, Apr 8

bjope added a comment to D59626: [AMDGPU] Add MachineDCE pass after RenameIndependentSubregs.

Bjorn,

did you enable MachineDCE in RA in some other target? So far it is only enabled in the AMDGPU, so I am curious where do you see these problems?

Mon, Apr 8, 11:19 AM · Restricted Project
bjope added a comment to D59626: [AMDGPU] Add MachineDCE pass after RenameIndependentSubregs.

I see a couple of problems with the changes to DeadMachineInstructionElim.cpp in this patch:

  • I get MachineVerifier problems because the LIS update does not take into consideration that the removal of a dead use can introduce disjoint live ranges (resulting in verifier complaining about "Multiple connected components in live interval".
  • I get MachineVerifier problems because the LIS update does not update live ranges for physical registers. Consider an instruction like this dead %42:regs = COPY $a1, if that is removed and it is the last use of the physical register $a1 then we need to shrink the live range for $a1 , otherwise the MachineVerifier will complain about "Live segment doesn't end at a valid instruction".
  • I do not think the !MRI->reg_empty(Reg) check takes debug info into consideration. Possibly introducing a debug invariance problem.
  • It does not seem like this pass is using the LIS itself. So why do we even bother keeping it update? It is destroyed, and then it is up to the pass manager to detect if it needs to be recalculated by a later pass, right? Or do we know that it will be used by a later pass, and thereby we want to bother about preserving it (even if it isn't for the greater good of the pass itself). If it is like that, then we probably need to add some code comments to highlight that we only do it because some other pass needs it (for example in case that pass is removed in the future).
  • Is it really true that we now preserve all possible analyses (both existing and future, both upstream and downstream), motivating the change from setPreservesCFG() to setPreservesAll(). I mean, the pass is pretty "destructive" as it removes instructions. You have only added (afaict incomplete) support to preserve the LIS. Notice that I'm not sure exactly what the difference between "PreservesAll" and "PreservesCFG" is, but I suppose it includes more analysis results besides LIS (and SlotIndices).
Mon, Apr 8, 4:25 AM · Restricted Project

Fri, Apr 5

bjope added a comment to D59790: [DebugInfo][Docs] Document how variable location metadata is transformed through target codegen.

One more thing that could be described (not neccessarily in this patch):

  • dbg.declare is removed by ISel and the information is squirreled away in a table in the MachineFunction. Later DwarfDebug::collectVariableInfoFromMFTable is fetching the debug info using MF->getVariableDbgInfo(). That kind of explains why there is no DBG_DECLARE meta instruction, or any DBG_VALUE instructions when using -O0, which otherwise could be a mystery for someone not familiar with that part.
Fri, Apr 5, 4:37 AM · Restricted Project
bjope added a comment to D60311: MIR printer should lowercase sub-register names to be in sync with parser?.

I think it would be better to let the printer use the same convention as the specified in the definitions (as used in the code).
And then I think it is a bug in the parser, as it tries to be case insensitive by matching the parsed string against a lower case definition. It should also lower the parsed string if we want the parser to be case insensitive.

Fri, Apr 5, 2:37 AM · Restricted Project

Wed, Apr 3

bjope updated the diff for D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.

Updated according to suggestion from @RKSimon, i.e. moving the combines only
done for ADD into the visitADD function.

Wed, Apr 3, 8:58 AM · Restricted Project

Mon, Apr 1

bjope updated the diff for D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.

Added extra test in test/CodeGen/X86/signbit-shift.ll to show that add_zext_ifpos_vec_splat only turn up as a regression due to register contraints.

Mon, Apr 1, 10:45 AM · Restricted Project

Thu, Mar 28

bjope added a comment to D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.

The Hexagon testcase can be fixed---it's probably just a matter of changing the selection pattern for the instruction we're checking.

Thu, Mar 28, 6:14 AM · Restricted Project

Wed, Mar 27

bjope added a comment to D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.

Hello reviewers! Do you think this is a good idea?

It's an interesting idea. :)

I've mostly seen improvements for our OOT target when doing this, but for example llvm/test/CodeGen/X86/split-store.ll also exposes a case when we trigger a rewrite into using SUB.

Yes, we'd classify that as a slight regression for x86.

Isn't split-store.ll showing an improvement (we get one subb instead of andb+orb)?

Yes - sorry, I reversed that with signbit-shift.ll. So we would call signbit-shift.ll a slight regression because of the extra mov instruction. We are probably missing a generic combine. Might be similar to the hexagon diff?

Wed, Mar 27, 11:08 AM · Restricted Project
bjope added a comment to D56151: [DebugInfo] PR40010: Avoid register coalesing altering DBG_VALUE valuations.
  • Is this patch really supposed to detect this DBG_VALUE as unsound? (we are not updating %43 here, we are only extending the live range to cover the DBG_VALUE that already is using %43). Should perhaps mergingChangesDbgValue return false also when (SrcLive && DstReg == DbgReg), or could this be unsound?
Wed, Mar 27, 4:54 AM
bjope added a comment to D56151: [DebugInfo] PR40010: Avoid register coalesing altering DBG_VALUE valuations.

I'll try to describe one "regression" that I've seen.

Wed, Mar 27, 3:03 AM

Tue, Mar 26

bjope added a comment to D56151: [DebugInfo] PR40010: Avoid register coalesing altering DBG_VALUE valuations.

FYI: I've downloaded this patch and have been running some tests (on a C code base). Lots of DBG_VALUE:s that now becomes undef. Some of them were totally wrong before (so this patch is good). But in some situations it looks like a regression. Need some more time to analyse those to find out if there is something wrong with mergingChangesDbgValue (that can be easily fixed), or to understand better why we get those regressions.

Tue, Mar 26, 9:06 AM
bjope added a comment to D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.

Hello reviewers! Do you think this is a good idea?

It's an interesting idea. :)

I've mostly seen improvements for our OOT target when doing this, but for example llvm/test/CodeGen/X86/split-store.ll also exposes a case when we trigger a rewrite into using SUB.

Yes, we'd classify that as a slight regression for x86.

Tue, Mar 26, 8:31 AM · Restricted Project

Mar 25 2019

bjope added a comment to D59272: [DebugInfo] Select debug intrinsic line-numbers more carefully when promoting dbg.declare.

Hi,

Isn't there a risk that this (partially) is hiding bugs in some other passes, where the DebugLoc is picked from the wrong place. I guess that either it should be picked from an adjacent non-meta instruction. Or it should be set to unknown.
With this patch we only eliminate the risk that the faulty passes are picking an "incorrect" DebugLoc from a dbg.value intrinsic (or DBG_VALUE instruction). But the "faulty" pass could still pick incorrect DebugLoc from other instructions, right? Or is it always correct to take the DebugLoc from all other non-dbg.value instructions, including all other kinds of meta-instructions?

Might be hard to find all such bugs. But with your known examples it shouldn't be too hard to find at least some places in opt/llc where the DebugLoc from the dbg.value is infecting some other instruction. Then perhaps we want to land this patch as well, just do avoid some not-yet-detected problems.

All true -- this patch can be characterised as simply reducing the risk of leaking definitely-wrong line numbers into non-meta IR instructions, which I think has some value.

Quickly testing what happens if IRBuilder is adjusted to not pick take DebugLocs from debug-intrinsics [0], roughly 750 of the 900 entries this patch changes, are also changed, possibly to different lines though. (I'll upload this trivial change sometime soon too).

That's likely the bulk of the leaking line numbers; while the rest could be hunted down, they're likely to be scattered through many places, so IMO this patch is the most time-efficient solution going forwards.

[0] https://github.com/llvm-mirror/llvm/blob/e3696113b639c8bf0b72d6c27dd76d6fdd8ebf61/include/llvm/IR/IRBuilder.h#L137

Mar 25 2019, 8:07 AM · Restricted Project
bjope added a reviewer for D59758: [DAGCombiner] Combine OR as ADD when no common bits are set: kparzysz.
Mar 25 2019, 2:53 AM · Restricted Project
bjope added a comment to D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.

Hello reviewers! Do you think this is a good idea?

Mar 25 2019, 2:53 AM · Restricted Project
bjope created D59758: [DAGCombiner] Combine OR as ADD when no common bits are set.
Mar 25 2019, 2:45 AM · Restricted Project

Mar 22 2019

bjope committed rG2f09ba541bf9: [KnownBits] Add const to some methods. NFC (authored by bjope).
[KnownBits] Add const to some methods. NFC
Mar 22 2019, 12:37 PM
bjope committed rL356797: [KnownBits] Add const to some methods. NFC.
[KnownBits] Add const to some methods. NFC
Mar 22 2019, 12:37 PM
bjope added a comment to D59272: [DebugInfo] Select debug intrinsic line-numbers more carefully when promoting dbg.declare.

Isn't there a risk that this (partially) is hiding bugs in some other passes, where the DebugLoc is picked from the wrong place. I guess that either it should be picked from an adjacent non-meta instruction. Or it should be set to unknown.
With this patch we only eliminate the risk that the faulty passes are picking an "incorrect" DebugLoc from a dbg.value intrinsic (or DBG_VALUE instruction). But the "faulty" pass could still pick incorrect DebugLoc from other instructions, right? Or is it always correct to take the DebugLoc from all other non-dbg.value instructions, including all other kinds of meta-instructions?

Mar 22 2019, 10:30 AM · Restricted Project
bjope added a comment to D58453: [DebugInfo][CGP] Limit placeDbgValues movement of dbg.value intrinsics.

What is the status regarding this patch? Are we waiting for something?

Mar 22 2019, 10:03 AM · Restricted Project

Mar 7 2019

bjope added a comment to D58238: [DebugInfo] MachineSink: Insert undef DBG_VALUEs when sinking instructions, try to forward copies.

I produced another patch (D59027, work-in-progress) that only creates undef DBG_VALUEs when the order of assignments would change... however I then realised I'd misunderstood Bjorns question here:

It might be that we sink past instructions that has been sunk earlier (or simply been re-scheduled at ISel etc), so it could be that we restore the source order when sinking. Or we could be sinking past some constant materialisation that has no associated source line. Etc.

After ISel we could have something like this:

%1 = LOAD ...
DBG_VALUE %1, %noreg, "x", ...
%2 = AND ...
%3 = ADD %1, 3
DBG_VALUE %3, %noreg, "x", ...
%5 = SUB 0, 0

or we could just as well have the SUB before the ADD afaict

%1 = LOAD ...
DBG_VALUE %1, %noreg, "x", ...
%2 = AND ...
%5 = SUB 0, 0
%3 = ADD %1, 3
DBG_VALUE %3, %noreg, "x", ...

Sinking the ADD past the SUB would introduce an undef location for "x", when done by MachineSink. But not if the instructions were emitted in that order already at ISel.

If I understand you correctly, this actually isn't a case that MachineSink deals with: MachineSink sinks insts from parent to child basic blocks, to remove partially redundant computations. Take this example [0], where the load of %ab is only used down one path, MachineSink moves it to only execute in the block which actually uses the load. With that in mind it might be clearer why undefs become necessary: if the DBG_VALUE moves to a different block, one of the source-level assignments to the corresponding variable has disappeared from other paths through the program. Those paths should have the variable appear ``optimised out'' until the variable gets a new location, to stop the old location being used on the other paths which would present a stale variable value (effectively this re-orders the appearance of assignments).

Using the example quoted above, if ADD were sunk into a _successor_ block, then an undef DBG_VALUE would have to go somewhere, to terminate the earlier location of "x" in the other successor blocks. (NB, MachineSink doesn't operate if there's only one successor).

[0] https://github.com/llvm-mirror/llvm/blob/master/test/CodeGen/X86/MachineSink-DbgValue.ll

Mar 7 2019, 4:08 PM · Restricted Project
bjope added inline comments to D55720: [Intrinsic] Signed Fixed Point Saturation Multiplication Intrinsic.
Mar 7 2019, 8:10 AM · Restricted Project

Mar 4 2019

bjope added inline comments to D58520: [Subtarget] Remove static global constructor call from the tablegened subtarget feature tables.
Mar 4 2019, 10:50 AM · Restricted Project
bjope added inline comments to D58520: [Subtarget] Remove static global constructor call from the tablegened subtarget feature tables.
Mar 4 2019, 2:27 AM · Restricted Project

Mar 1 2019

bjope added inline comments to D58831: [DebugInfo] Ignore bitcasts when lowering stack arg dbg.values.
Mar 1 2019, 8:39 AM · Restricted Project, debug-info
bjope added inline comments to D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions.
Mar 1 2019, 12:19 AM · Restricted Project

Feb 28 2019

bjope added inline comments to D58650: Add support for computing "zext of value" in KnownBits. NFCI.
Feb 28 2019, 10:49 AM · Restricted Project
bjope committed rGd30f308a9ff9: Add support for computing "zext of value" in KnownBits. NFCI (authored by bjope).
Add support for computing "zext of value" in KnownBits. NFCI
Feb 28 2019, 7:45 AM
bjope committed rL355099: Add support for computing "zext of value" in KnownBits. NFCI.
Add support for computing "zext of value" in KnownBits. NFCI
Feb 28 2019, 7:45 AM
bjope closed D58650: Add support for computing "zext of value" in KnownBits. NFCI.
Feb 28 2019, 7:44 AM · Restricted Project

Feb 27 2019

bjope added inline comments to D58726: [DebugInfo][Docs] Explicitly document how dbg.value intrinsics are interpreted in optimized code.
Feb 27 2019, 11:19 AM · Restricted Project
bjope added inline comments to D58650: Add support for computing "zext of value" in KnownBits. NFCI.
Feb 27 2019, 6:04 AM · Restricted Project
bjope added a reviewer for D58650: Add support for computing "zext of value" in KnownBits. NFCI: RKSimon.
Feb 27 2019, 2:25 AM · Restricted Project
bjope added inline comments to D57883: [clang-tidy] refactor ExceptionAnalyzer further to give ternary answer.
Feb 27 2019, 2:07 AM · Restricted Project
bjope added inline comments to D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions.
Feb 27 2019, 1:15 AM · Restricted Project

Feb 26 2019

bjope added a comment to D58453: [DebugInfo][CGP] Limit placeDbgValues movement of dbg.value intrinsics.

It is definitely about time that we also refresh the documentation. One thing that is lacking is documentation of the backend parts (currently https://llvm.org/docs/SourceLevelDebugging.html talks about intrinsics like dbg.value etc, but shouldn't it also cover DBG_VALUE pseudo instruction). Afaict dbg.value and DBG_VALUE aren't exactly the same. And I think even DBG_VALUE might have some different properties depending on where in the llc pipeline we are (e.g. if the location only is valid up to the end of the BB or not).

Feb 26 2019, 1:02 AM · Restricted Project

Feb 25 2019

bjope updated the diff for D58650: Add support for computing "zext of value" in KnownBits. NFCI.

Minor cleanup.

Feb 25 2019, 4:02 PM · Restricted Project
bjope created D58650: Add support for computing "zext of value" in KnownBits. NFCI.
Feb 25 2019, 3:30 PM · Restricted Project
bjope updated subscribers of D57883: [clang-tidy] refactor ExceptionAnalyzer further to give ternary answer.

Maybe @sammccall remembers why it was decided to rewrite the enum into constexpr:s in https://reviews.llvm.org/rCTE319608 ? Do we need a similar solution here?

Feb 25 2019, 2:38 AM · Restricted Project
bjope updated subscribers of D57883: [clang-tidy] refactor ExceptionAnalyzer further to give ternary answer.
Feb 25 2019, 2:36 AM · Restricted Project

Feb 22 2019

bjope added inline comments to D55720: [Intrinsic] Signed Fixed Point Saturation Multiplication Intrinsic.
Feb 22 2019, 4:28 PM · Restricted Project
bjope added a comment to D58453: [DebugInfo][CGP] Limit placeDbgValues movement of dbg.value intrinsics.

placeDbgValues has been bugging me (and probably others) for some time. So I'm happy with this patch.

Feb 22 2019, 5:15 AM · Restricted Project
bjope added a comment to D58238: [DebugInfo] MachineSink: Insert undef DBG_VALUEs when sinking instructions, try to forward copies.

Maybe it is a philosophical question. Is the compiler scheduling/reordering source instructions (in the debugger it will appear as source statements are executed in a random order), or are we scheduling/reordering machine instructions (and in the debugger it still should appear as if we execute variable assignments in the order of the source code). VLIW-scheduling, tail merging, etc, of course make things extra complicated, since we basically will be all over the place all the time.

To me it's the latter, my mental model (maybe wrong) is that while the compiler is optimising code, for debuginfo it has a state-machine of variable locations represented by dbg.values that it needs to try and preserve the order of, marking locations undef if they don't exist any more. The logical consequence is that if we had a function where the compiler managed to completely reverse the order of computation, no variable would have a location, which would be sound, but not useful.

Feb 22 2019, 1:56 AM · Restricted Project

Feb 21 2019

bjope added inline comments to D58453: [DebugInfo][CGP] Limit placeDbgValues movement of dbg.value intrinsics.
Feb 21 2019, 3:31 PM · Restricted Project

Feb 20 2019

bjope added inline comments to D58403: [DebugInfo][CGP] Update dbg.values when updating memory address computations.
Feb 20 2019, 12:17 AM · Restricted Project

Feb 15 2019

bjope added a comment to D58238: [DebugInfo] MachineSink: Insert undef DBG_VALUEs when sinking instructions, try to forward copies.

I have some doubt about this. Mainly the part about inserting undefs. Although, some of my doubts are partially based on the big-black-hole regarding how debug info is supposed to work for heavily optimized code (and more specifically how this is supposed to work in LLVM based on what we got today).

Feb 15 2019, 2:54 AM · Restricted Project