bjope (Bjorn Pettersson)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 26 2016, 7:58 AM (86 w, 4 d)

Recent Activity

Tue, May 22

bjope added a comment to D47199: [MC] Remove PhysRegSize from MCRegisterClass.

Is this cleanup OK now?
I know that @kparzysz tried this originally in https://reviews.llvm.org/D31299, but that refactoring ended up leaving these methods and the PhysRegSize member around.

Tue, May 22, 8:55 AM
bjope created D47199: [MC] Remove PhysRegSize from MCRegisterClass.
Tue, May 22, 8:45 AM
bjope added inline comments to rC332950: GNUstep Objective-C ABI version 2.
Tue, May 22, 2:30 AM
bjope added inline comments to rC332950: GNUstep Objective-C ABI version 2.
Tue, May 22, 1:47 AM
bjope committed rL332958: [LoopVersioning] Don't modify the list that we iterate over in addPHINodes.
[LoopVersioning] Don't modify the list that we iterate over in addPHINodes
Tue, May 22, 1:37 AM
bjope closed D47134: [LoopVersioning] Don't modify the list that we iterate over in addPHINodes.
Tue, May 22, 1:36 AM
bjope added inline comments to rC332950: GNUstep Objective-C ABI version 2.
Tue, May 22, 1:34 AM
bjope committed rC332957: Revert r332955 "GNUstep Objective-C ABI version 2".
Revert r332955 "GNUstep Objective-C ABI version 2"
Tue, May 22, 1:21 AM
bjope committed rL332957: Revert r332955 "GNUstep Objective-C ABI version 2".
Revert r332955 "GNUstep Objective-C ABI version 2"
Tue, May 22, 1:20 AM
bjope added inline comments to rC332950: GNUstep Objective-C ABI version 2.
Tue, May 22, 12:56 AM

Mon, May 21

bjope added inline comments to rC332950: GNUstep Objective-C ABI version 2.
Mon, May 21, 11:58 PM
bjope updated the diff for D47134: [LoopVersioning] Don't modify the list that we iterate over in addPHINodes.

Updated according to suggestions from @mzolotukhin.
Thanks for the review (I'll push this tomorrow when I can observe the build bots).

Mon, May 21, 1:27 PM
bjope created D47134: [LoopVersioning] Don't modify the list that we iterate over in addPHINodes.
Mon, May 21, 4:05 AM

Thu, May 17

bjope committed rL332577: [SROA] Handle PHI with multiple duplicate predecessors.
[SROA] Handle PHI with multiple duplicate predecessors
Thu, May 17, 12:26 AM
bjope closed D46426: [SROA] Handle PHI with multiple duplicate predecessors.
Thu, May 17, 12:26 AM

Wed, May 16

bjope added a comment to D46426: [SROA] Handle PHI with multiple duplicate predecessors.

Thanks Eli. Definitely a cleaner solution when using a DenseMap.

Wed, May 16, 1:57 PM
bjope updated the diff for D46426: [SROA] Handle PHI with multiple duplicate predecessors.

Replaced the unordered_map by a DenseMap.

Wed, May 16, 1:55 PM
bjope added inline comments to D46426: [SROA] Handle PHI with multiple duplicate predecessors.
Wed, May 16, 1:18 PM
bjope added a comment to D46426: [SROA] Handle PHI with multiple duplicate predecessors.

So the patch has been confirmed to solve the PR (thanks @uabelho).
But still no ready-to-land. Well I think that I'll land this anyway tomorrow, to get rid of the bug.

Wed, May 16, 11:58 AM
bjope updated subscribers of rL332379: AMDGPU/GlobalISel: Implement select() for G_FCONSTANT.
Wed, May 16, 2:50 AM
bjope updated subscribers of rL332379: AMDGPU/GlobalISel: Implement select() for G_FCONSTANT.
Wed, May 16, 2:49 AM

Mon, May 14

bjope added a comment to D46426: [SROA] Handle PHI with multiple duplicate predecessors.

ping!

Mon, May 14, 1:04 PM

Fri, May 11

bjope accepted D46739: [DebugInfo] Only handle DBG_VALUE in InlineSpiller.

Thanks! LGTM

Fri, May 11, 2:45 PM
bjope added a reviewer for D46426: [SROA] Handle PHI with multiple duplicate predecessors: bkramer.
Fri, May 11, 2:41 PM

Thu, May 10

bjope accepted D46668: [STLExtras] Add distance() for ranges, pred_size(), and succ_size().

Nice, I was looking for something like this when updating MergedLoadStoreMotion.cpp.

Thu, May 10, 3:18 PM

Wed, May 9

bjope added inline comments to D45342: [DebugInfo] Examine all uses of isDebugValue() for debug instructions..
Wed, May 9, 1:15 PM

Tue, May 8

bjope committed rL331852: [MergedLoadStoreMotion] Fix a debug invariant bug in mergeStores.
[MergedLoadStoreMotion] Fix a debug invariant bug in mergeStores
Tue, May 8, 11:56 PM
bjope closed D46600: [MergedLoadStoreMotion] Fix a debug invariant bug in mergeStores.
Tue, May 8, 11:56 PM
bjope added inline comments to D46600: [MergedLoadStoreMotion] Fix a debug invariant bug in mergeStores.
Tue, May 8, 2:22 PM
bjope added a comment to D46600: [MergedLoadStoreMotion] Fix a debug invariant bug in mergeStores.

Thanks alot, that was a quick review!
(I'll land this tomorrow when I have time to monitor the buildbots)

Tue, May 8, 1:54 PM
bjope created D46600: [MergedLoadStoreMotion] Fix a debug invariant bug in mergeStores.
Tue, May 8, 1:46 PM
bjope committed rL331741: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.
[LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions
Tue, May 8, 12:03 AM
bjope closed D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.
Tue, May 8, 12:03 AM

Mon, May 7

bjope updated subscribers of D45842: [Reassociate] swap binop operands to increase factoring potential.
Mon, May 7, 11:55 PM
bjope retitled D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions from [LCSSA] Iteratively remove unused PHI nodes in formLCSSAForInstructions to [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.
Mon, May 7, 10:45 AM
bjope updated the diff for D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.

Simplified the solution, assuming that it is ok to leave some unused PHI nodes behind, at least if the input contains unreachable basic blocks.

Mon, May 7, 10:45 AM
bjope added a comment to D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.

I looked deeper into this, and I think you exposed several issues here:

  • The entire thing is triggered by the fact that the value we're rewriting is used in an unreachable block. We might just ignore such cases (see the loop on lines 102-110).
  • The loops in this test are not in simplified form. While they are not guaranteed to be in it, it might indicate that we have a bug in some other pass that fails to preserve the loop in a simplified form. Whatever fix for the current issue we choose, it would be interesting to understand what led to this situation.
Mon, May 7, 10:20 AM
bjope added a comment to D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.

A few notes:

  1. Because there is no single root here reaching all nodes, you can't guarantee the ordering will give a single iteration order for any sort order of the list.
  2. We are assuming no cycles (you could find sccs here to deal with the cycles, it's the same cost as finding RPO because they are done the same way)
  3. Off the top of my head, my intuition is that if the list is in dominator tree order (which is an RPO except for siblings) to start, the RPO generated by DFS of the operand tree should avoid the problem mentioned in 1, because it would initially order it the same way it would be ordered in the presence of a single root.
Mon, May 7, 8:26 AM

Fri, May 4

bjope added a comment to D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.

Is it guaranteed that a PHI node, without uses, being inserted in PHIsToRemove only can be used by later created PHI nodes

PHI-nodes are only inserted when we call SSAUpdater.RewriteUse. I did not expected that their use lists change afterwards, so frankly, I'm surprised that inserted PHIs get more uses on later iterations - that's why I'm asking for a simpler test that could help to understand how and why this happens.

Then it might be enough to iterate in reverse insertion order...

Why do we need to reverse the order?

Fri, May 4, 4:14 PM
bjope added a comment to D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.

Is there an intermediate IR dump you can send me of where you end up with
phis using phis that are useless?

(I'd also like to understand if you ever get cycles of that happening as
well)

Fri, May 4, 3:56 PM
bjope added a comment to D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.

With the new test case the method formLCSSAForInstructions is invoked three times (three loops?).

Fri, May 4, 3:46 PM
bjope updated the diff for D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.

Updated the test case (elimintated more branches/blocks, renamed variables/labels, removed checks).

Fri, May 4, 3:26 PM
bjope added a comment to D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.

Hi,

First of all, please clean-up the testcase. The bugpoint-output tests are impossible to understand and maintain (it is impossible to say if such a test checks anything at all after some time). I'm pretty sure you can expose the same issue with much a smaller test.

Fri, May 4, 12:40 PM
bjope added inline comments to D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.
Fri, May 4, 12:33 PM
bjope updated subscribers of D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.
Fri, May 4, 5:30 AM
bjope created D46426: [SROA] Handle PHI with multiple duplicate predecessors.
Fri, May 4, 5:30 AM
bjope added inline comments to D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.
Fri, May 4, 3:56 AM
bjope created D46422: [LCSSA] Do not remove used PHI nodes in formLCSSAForInstructions.
Fri, May 4, 3:55 AM
bjope added a comment to D45842: [Reassociate] swap binop operands to increase factoring potential.

As far as I can see this is another case of reusing existing Instructions/Values, while changing the actual value that the Instruction produce, right?

Take a look at the debug-info fixes I made here for a similar problem in RewriteExprTree: https://reviews.llvm.org/D45975
I suspect that you may need to discard debug-info in a similar way as in D45975, somewhere inside your new function swapOperandsToMatchBinops.

Thanks! You're correct that we're recycling instructions here (not sure why we don't just create new instructions with a Builder?). The funny thing about all the examples here is that after we swap operands in swapOperandsToMatchBinops(), we end up going through the main reassociation loop again and make more changes which triggers your code D45975, so I already see 'undef' in the right places.

Nevertheless, it's a small change to extract and call that code, so let me do that to be safe.

Fri, May 4, 3:53 AM
bjope committed rL331510: [SelectionDAG] Refactor code by adding RegsForValue::getRegsAndSizes(). NFCI.
[SelectionDAG] Refactor code by adding RegsForValue::getRegsAndSizes(). NFCI
Fri, May 4, 1:54 AM
bjope closed D46360: [SelectionDAG] Refactor code by adding RegsForValue::getRegsAndSizes(). NFCI.
Fri, May 4, 1:54 AM

Thu, May 3

bjope updated the diff for D46360: [SelectionDAG] Refactor code by adding RegsForValue::getRegsAndSizes(). NFCI.

Rebased after rL331464

Thu, May 3, 11:52 AM
bjope added inline comments to D46384: Reapply "[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)".
Thu, May 3, 11:42 AM
bjope committed rL331465: [DebugInfo] Correction for an assert in DIExpression::createFragmentExpression.
[DebugInfo] Correction for an assert in DIExpression::createFragmentExpression
Thu, May 3, 10:08 AM
bjope committed rL331464: Reapply "[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)".
Reapply "[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)"
Thu, May 3, 10:08 AM
bjope closed D46391: [DebugInfo] Correction for an assert in DIExpression::createFragmentExpression.
Thu, May 3, 10:08 AM
bjope closed D46384: Reapply "[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)".
Thu, May 3, 10:07 AM
bjope added inline comments to D46391: [DebugInfo] Correction for an assert in DIExpression::createFragmentExpression.
Thu, May 3, 9:47 AM
bjope added a comment to D46391: [DebugInfo] Correction for an assert in DIExpression::createFragmentExpression.

Does this fire anywhere? If so, we should try adding a test.

Thu, May 3, 8:18 AM
bjope created D46391: [DebugInfo] Correction for an assert in DIExpression::createFragmentExpression.
Thu, May 3, 7:29 AM
bjope updated the summary of D46384: Reapply "[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)".
Thu, May 3, 7:06 AM
bjope updated the diff for D46384: Reapply "[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)".

Updated to handle more situations (when the original fragment is so small, so we do not even need to use all parts of the split phi node).
Since we now need to be aware about the size of the old fragment at the call to createFragmentExpression I decided to adjust the size already at the caller and not inside createFragmentExpression.

Thu, May 3, 7:06 AM
bjope updated the diff for D46384: Reapply "[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)".

Just a minor change to enhance readability.

Thu, May 3, 5:06 AM
bjope created D46384: Reapply "[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)".
Thu, May 3, 4:46 AM
bjope added a comment to D46329: [SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2).

Unfortunately, this broke things for me, see https://bugs.llvm.org/show_bug.cgi?id=37321 for details. I'll revert this one in the meantime.

Thu, May 3, 12:25 AM · debug-info

Wed, May 2

bjope created D46360: [SelectionDAG] Refactor code by adding RegsForValue::getRegsAndSizes(). NFCI.
Wed, May 2, 10:29 AM
bjope committed rL331337: [SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2).
[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)
Wed, May 2, 12:00 AM
bjope closed D46329: [SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2).
Wed, May 2, 12:00 AM · debug-info

Tue, May 1

bjope updated the diff for D46329: [SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2).

Some updates suggested by vsk:

  • Use zip_first.
  • Add a RegsForValue::occupiesMultipleRegisters() helper.
  • Cleanup in the test case (removing tbaa and lifetime stuff).
Tue, May 1, 2:14 PM · debug-info
bjope added inline comments to D46329: [SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2).
Tue, May 1, 1:33 PM · debug-info
bjope created D46329: [SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2).
Tue, May 1, 12:51 PM · debug-info

Mon, Apr 30

bjope added inline comments to D45878: [DEBUG INFO] Fixing cases where debug info (-g) causes changes in the program..
Mon, Apr 30, 7:50 AM
bjope committed rL331182: [SelectionDAG] Improve selection of DBG_VALUE using a PHI node result.
[SelectionDAG] Improve selection of DBG_VALUE using a PHI node result
Mon, Apr 30, 7:41 AM
bjope committed rL331183: [BranchFolding] Salvage DBG_VALUE instructions from empty blocks.
[BranchFolding] Salvage DBG_VALUE instructions from empty blocks
Mon, Apr 30, 7:41 AM
bjope closed D46129: [SelectionDAG] Improve selection of DBG_VALUE using a PHI node result.
Mon, Apr 30, 7:41 AM
bjope closed D46184: [BranchFolding] Salvage DBG_VALUE instructions from empty blocks.
Mon, Apr 30, 7:41 AM

Fri, Apr 27

bjope added a comment to D46184: [BranchFolding] Salvage DBG_VALUE instructions from empty blocks.

Have you thought about just changing the scheduling of the LiveDebugValues pass to run earlier instead? Would that be an overall improvement?

Fri, Apr 27, 9:31 AM
bjope added a comment to D45842: [Reassociate] swap binop operands to increase factoring potential.

As far as I can see this is another case of reusing existing Instructions/Values, while changing the actual value that the Instruction produce, right?

Fri, Apr 27, 7:39 AM
bjope added inline comments to D46129: [SelectionDAG] Improve selection of DBG_VALUE using a PHI node result.
Fri, Apr 27, 6:29 AM
bjope created D46184: [BranchFolding] Salvage DBG_VALUE instructions from empty blocks.
Fri, Apr 27, 5:35 AM
bjope updated the summary of D46129: [SelectionDAG] Improve selection of DBG_VALUE using a PHI node result.
Fri, Apr 27, 5:12 AM

Thu, Apr 26

bjope added inline comments to D46129: [SelectionDAG] Improve selection of DBG_VALUE using a PHI node result.
Thu, Apr 26, 12:08 PM
bjope added inline comments to D46129: [SelectionDAG] Improve selection of DBG_VALUE using a PHI node result.
Thu, Apr 26, 9:24 AM
bjope created D46129: [SelectionDAG] Improve selection of DBG_VALUE using a PHI node result.
Thu, Apr 26, 9:16 AM

Apr 25 2018

bjope created D46059: [Scalarizer] Remove unreachable blocks before scalarizing a function.
Apr 25 2018, 6:46 AM
bjope created D46053: [Local] Refactor llvm::removeUnreachableBlocks.
Apr 25 2018, 5:22 AM
bjope committed rL330804: [DebugInfo] Invalidate debug info in ReassociatePass::RewriteExprTree.
[DebugInfo] Invalidate debug info in ReassociatePass::RewriteExprTree
Apr 25 2018, 2:27 AM
bjope closed D45975: [DebugInfo] Invalidate debug info in ReassociatePass::RewriteExprTree.
Apr 25 2018, 2:27 AM · debug-info
bjope committed rL330802: Fix buildbot problems after rC330794.
Fix buildbot problems after rC330794
Apr 25 2018, 2:07 AM
bjope committed rC330802: Fix buildbot problems after rC330794.
Fix buildbot problems after rC330794
Apr 25 2018, 2:07 AM

Apr 24 2018

bjope updated the diff for D45975: [DebugInfo] Invalidate debug info in ReassociatePass::RewriteExprTree.

Thanks Adrian! Updated according to your suggestion and this should be more correct.

Apr 24 2018, 8:34 AM · debug-info

Apr 23 2018

bjope committed rL330635: [MemCpyOpt] Skip optimizing basic blocks not reachable from entry.
[MemCpyOpt] Skip optimizing basic blocks not reachable from entry
Apr 23 2018, 12:58 PM
bjope closed D45889: [MemCpyOpt] Skip optimizing basic blocks not reachable from entry.
Apr 23 2018, 12:58 PM
bjope added inline comments to D45975: [DebugInfo] Invalidate debug info in ReassociatePass::RewriteExprTree.
Apr 23 2018, 12:09 PM · debug-info
bjope added inline comments to D45975: [DebugInfo] Invalidate debug info in ReassociatePass::RewriteExprTree.
Apr 23 2018, 11:41 AM · debug-info
bjope created D45975: [DebugInfo] Invalidate debug info in ReassociatePass::RewriteExprTree.
Apr 23 2018, 11:13 AM · debug-info
bjope added a comment to D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature.

buildbots have been failing all day (and were still failing). So I pushed my suggested fix.
I hope that was OK.

Apr 23 2018, 11:10 AM
bjope committed rC330622: Improve checks in test/Frontend/ftime-report-template-decl.cpp.
Improve checks in test/Frontend/ftime-report-template-decl.cpp
Apr 23 2018, 11:08 AM
bjope committed rL330622: Improve checks in test/Frontend/ftime-report-template-decl.cpp.
Improve checks in test/Frontend/ftime-report-template-decl.cpp
Apr 23 2018, 11:08 AM
bjope added a comment to D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature.

I can't see that it has been reverted.
But I guess that the table maybe is sorted based on time spent in each pass? So that is why it might be sorted differently on different buildbots (or when using pipe etc).

Apr 23 2018, 8:11 AM