bjope (Bjorn Pettersson)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 26 2016, 7:58 AM (99 w, 10 h)

Recent Activity

Today

bjope added inline comments to D50621: [DebugInfo] Fix bug in LiveDebugVariables..
Mon, Aug 20, 12:29 AM

Fri, Aug 17

bjope added inline comments to D50638: [AsmPrinter] Look inside bundles and rely on FrameDestroy in calculateDbgValueHistory.
Fri, Aug 17, 5:22 AM

Thu, Aug 16

bjope abandoned D46059: [Scalarizer] Remove unreachable blocks before scalarizing a function.

I've got another solution for this problem for our out-of-tree-target. So I'll abandon this.

Thu, Aug 16, 9:22 AM
bjope abandoned D46053: [Local] Refactor llvm::removeUnreachableBlocks.

I was going to use this in another patch, but it's outdated now so I'll abandon this.

Thu, Aug 16, 9:21 AM
bjope added a reviewer for D50639: Change how finalizeBundle selects debug location for the BUNDLE instruction: debug-info.
Thu, Aug 16, 9:19 AM
bjope added a reviewer for D50638: [AsmPrinter] Look inside bundles and rely on FrameDestroy in calculateDbgValueHistory: debug-info.
Thu, Aug 16, 9:19 AM
bjope added a reviewer for D50637: [CodeGen] Set FrameSetup/FrameDestroy on BUNDLE instructions: debug-info.
Thu, Aug 16, 9:18 AM
bjope created D50844: [RegisterCoalescer] Use substPhysReg in reMaterializeTrivialDef.
Thu, Aug 16, 7:47 AM
bjope added reviewers for D50842: [RegisterCoalescer] Do not assert when trying to remat dead values: kparzysz, wmi, tpr.

Just adding some of the latest commiters to RegisterCoalescer.cpp as reviewers. I hope someone is able to review this.

Thu, Aug 16, 7:15 AM
bjope created D50842: [RegisterCoalescer] Do not assert when trying to remat dead values.
Thu, Aug 16, 7:12 AM

Wed, Aug 15

bjope created D50788: [LiveDebugVariables] Avoid faulty addDefsFromCopies in computeIntervals.
Wed, Aug 15, 10:08 AM · debug-info

Mon, Aug 13

bjope created D50639: Change how finalizeBundle selects debug location for the BUNDLE instruction.
Mon, Aug 13, 8:22 AM
bjope created D50638: [AsmPrinter] Look inside bundles and rely on FrameDestroy in calculateDbgValueHistory.
Mon, Aug 13, 8:20 AM
bjope created D50637: [CodeGen] Set FrameSetup/FrameDestroy on BUNDLE instructions.
Mon, Aug 13, 8:20 AM

Fri, Aug 10

bjope added a comment to D50285: [MC] Remove MCRegisterClass::getSize.

What exactly are out of tree targets supposed to migrate to?

Fri, Aug 10, 1:39 AM

Thu, Aug 9

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

Accepting as per comments in D50285.

Thu, Aug 9, 8:28 AM
bjope committed rL339350: [MC] Remove PhysRegSize from MCRegisterClass.
[MC] Remove PhysRegSize from MCRegisterClass
Thu, Aug 9, 8:19 AM
bjope closed D47199: [MC] Remove PhysRegSize from MCRegisterClass.
Thu, Aug 9, 8:19 AM

Sun, Aug 5

bjope added a comment to D50285: [MC] Remove MCRegisterClass::getSize.

I think that it is about time to at least remove MCRegisterClass::getSize().
Notice that I also suggested to remove getPhysRegSize() (and the PhysRegSize member) in https://reviews.llvm.org/D47199, but it is perhaps more controversial to remove the register size completely from MCRegisterClass?
Afaik there is not in-tree use of getPhysRegSize(). And making it possible to get the size in bytes of a reg class with non-byte-sized registers, without telling that it truncates down to whole bytes, might be a little bit confusing (e.g. getPhysRegSize() for a register class with 1-bit registers will return 0).

Sun, Aug 5, 11:50 AM

Tue, Jul 31

bjope added inline comments to D49847: [SystemZ] Improve decoding in case of instructions with four register operands..
Tue, Jul 31, 10:34 AM

Jul 20 2018

bjope added inline comments to D49572: [docs] Clarify role of DIExpressions within debug intrinsics.
Jul 20 2018, 6:56 AM · debug-info

Jul 11 2018

bjope added a comment to D47541: Allow creating llvm::Function in non-zero address spaces.

This seems to work quite nice for me now! Nice!

Jul 11 2018, 4:19 AM

Jul 10 2018

bjope added inline comments to D48176: [AMDGPU] Refactor HSAMetadataStream::emitKernel (NFC).
Jul 10 2018, 3:42 PM
bjope committed rL336717: Patch to fix pragma metadata for do-while loops.
Patch to fix pragma metadata for do-while loops
Jul 10 2018, 1:00 PM
bjope committed rC336717: Patch to fix pragma metadata for do-while loops.
Patch to fix pragma metadata for do-while loops
Jul 10 2018, 1:00 PM
bjope closed D48721: Patch to fix pragma metadata for do-while loops.
Jul 10 2018, 1:00 PM · Restricted Project
bjope added a comment to D48721: Patch to fix pragma metadata for do-while loops.

@Bjorn, Thanks for reviewing and accepting the patch.

Could you please advise on the next steps?
Would someone else commit this on my behalf or should I request for commit access?

Thanks,
Deepak Panickal

Jul 10 2018, 12:24 PM · Restricted Project
bjope accepted D48721: Patch to fix pragma metadata for do-while loops.

LGTM!

Jul 10 2018, 9:24 AM · Restricted Project

Jul 9 2018

bjope added inline comments to D47541: Allow creating llvm::Function in non-zero address spaces.
Jul 9 2018, 10:56 AM

Jul 6 2018

bjope added a comment to D48676: [Local] replaceAllDbgUsesWith: Update debug values before RAUW.
In D48676#1153642, @vsk wrote:

FYI: Looks like you have address my earlier comments. So I have no blocking remarks.

Thanks for taking a look, I really appreciate your detailed feedback.

The addition of DomPoint / DT was interesting (not sure that I have figured out exactly which scenarios that are handled)

The two scenarios are:

  1. The replacement value is a Constant, will be inserted at a position which dominates the original instruction, or will be inserted immediately after the original instruction (which is deleted later). In this scenario there's no risk of introducing debug value use-before-def.
  2. The original value dominates the position where the replacement value will be inserted. A good example of this is the 'alloca-cast-debuginfo.ll' test. In an intermediate step, InstCombine inserts a temporary cast of an alloca:

    ` %local = alloca %struct.Foo %tmpcast = bitcast %local call dbg.declare(%tmpcast, "local") <some instructions follow> %castOfTmpcast = bitcast %tmpcast `

    InstCombine recognizes that it can remove 'tmpcast' and simplify 'castOfTmpcast' if it promotes the type of the alloca. Since 'tmpcast' has only one use, we try to save its debug users, anticipating that 'tmpcast' will be deleted. The DomPoint / DT machinery gives us a way to prevent this use-before-def for the unlinked 'simplifiedCast':

    ` %local = alloca i64 call dbg.declare(%simplifiedCast, "local") <some instructions follow> %simplifiedCast = bitcast %local `

    We either salvage the debug user or delete it. The unit test has examples of both situations.

I think that we still have the problem with only checking dominance of the moved/replaced value though.
As was discussed earlier somewhere (a patch in post RA MachineSink) we also might have the problem that there are other dbg.value intrinsics, referring to the same variable (or an overlapping fragment). And the question is if it is correct to reorder such dbg.value intrinsics, or what to do. Matt wrote a TR about it here https://bugs.llvm.org/show_bug.cgi?id=37897

Doesn't this patch sidestep issues with reordering variable updates, because the updates are now all done in-place?

Jul 6 2018, 12:47 AM

Jul 5 2018

bjope added a comment to D48676: [Local] replaceAllDbgUsesWith: Update debug values before RAUW.

FYI: Looks like you have address my earlier comments. So I have no blocking remarks.

Jul 5 2018, 11:28 AM
bjope added a comment to D48721: Patch to fix pragma metadata for do-while loops.

I have updated the test to not run the optimizer. The test I had added previously for checking if the unroller is respecting the pragma is useful I think. Not sure where that can be added though.
I guess it's independent of this patch anyway. If the patch and test is okay, will update bugzilla as well.

Jul 5 2018, 10:14 AM · Restricted Project
bjope added inline comments to D48721: Patch to fix pragma metadata for do-while loops.
Jul 5 2018, 8:12 AM · Restricted Project

Jul 4 2018

bjope added inline comments to D47541: Allow creating llvm::Function in non-zero address spaces.
Jul 4 2018, 5:21 AM

Jul 3 2018

bjope added inline comments to D47541: Allow creating llvm::Function in non-zero address spaces.
Jul 3 2018, 9:07 AM
bjope committed rL336194: [IR] Strip trailing whitespace. NFC.
[IR] Strip trailing whitespace. NFC
Jul 3 2018, 5:44 AM
bjope committed rL336191: [DebugInfo] Corrections for salvageDebugInfo.
[DebugInfo] Corrections for salvageDebugInfo
Jul 3 2018, 4:33 AM
bjope closed D48837: [DebugInfo] Corrections for salvageDebugInfo.
Jul 3 2018, 4:33 AM
bjope added a comment to D48721: Patch to fix pragma metadata for do-while loops.

Is the fault that the metadata only should be put on the back edge, not the branch in the preheader?

Yea. Our past thinking has been that any backedge in the loop is valid. The metadata shouldn't end up other places, although it's benign unless those other places are (or may later become) a backedge for some different loop.

Jul 3 2018, 4:19 AM · Restricted Project
bjope added a comment to D48780: [llvm-exegesis] Add an AArch64 target.

Fixed in r336188.

John

Jul 3 2018, 4:01 AM
bjope added a comment to D48780: [llvm-exegesis] Add an AArch64 target.

Can't build unittests (I suspect this patch):

Jul 3 2018, 3:51 AM

Jul 2 2018

bjope added a comment to D48721: Patch to fix pragma metadata for do-while loops.

I tried running

Jul 2 2018, 5:20 PM · Restricted Project
bjope added inline comments to D47541: Allow creating llvm::Function in non-zero address spaces.
Jul 2 2018, 4:38 PM
bjope added inline comments to D48837: [DebugInfo] Corrections for salvageDebugInfo.
Jul 2 2018, 9:54 AM
bjope created D48837: [DebugInfo] Corrections for salvageDebugInfo.
Jul 2 2018, 9:25 AM
bjope abandoned D48835: [DebugInfo].

Oops, not quite ready yet.

Jul 2 2018, 9:18 AM
bjope created D48835: [DebugInfo].
Jul 2 2018, 9:17 AM
bjope added a comment to D48803: Place the BlockAddress type in the program address space.

I don't know much about the BlockAddress concept. The LangRef says things like "always has an i8* type" and "this may be passed around as an opaque pointer sized value". But I guess it would be weird if the size doesn't match the size of pointers in the program address space, so the patch makes sense to me.

Jul 2 2018, 5:43 AM
bjope added inline comments to D48593: Reuse code for calculating allocation size for alloca.
Jul 2 2018, 4:42 AM

Jun 28 2018

bjope added inline comments to D48676: [Local] replaceAllDbgUsesWith: Update debug values before RAUW.
Jun 28 2018, 3:12 PM
bjope added a comment to D48331: [DebugInfo][InstCombine] Preserve DI after combining zext instructions.

Incorporate the -debugify test into the existing test files.

Jun 28 2018, 8:20 AM
bjope added inline comments to D48676: [Local] replaceAllDbgUsesWith: Update debug values before RAUW.
Jun 28 2018, 7:39 AM

Jun 27 2018

bjope added inline comments to D48331: [DebugInfo][InstCombine] Preserve DI after combining zext instructions.
Jun 27 2018, 11:56 AM
bjope added inline comments to D48331: [DebugInfo][InstCombine] Preserve DI after combining zext instructions.
Jun 27 2018, 11:26 AM
bjope added inline comments to D48331: [DebugInfo][InstCombine] Preserve DI after combining zext instructions.
Jun 27 2018, 11:17 AM
bjope added inline comments to D48331: [DebugInfo][InstCombine] Preserve DI after combining zext instructions.
Jun 27 2018, 10:02 AM

Jun 26 2018

bjope created D48593: Reuse code for calculating allocation size for alloca.
Jun 26 2018, 7:11 AM

Jun 25 2018

bjope committed rL335580: Improve ConvertDebugDeclareToDebugValue.
Improve ConvertDebugDeclareToDebugValue
Jun 25 2018, 11:21 PM
bjope closed D48547: Improve ConvertDebugDeclareToDebugValue.
Jun 25 2018, 11:21 PM
bjope updated the diff for D48547: Improve ConvertDebugDeclareToDebugValue.

Update after review comments:

  • Use Optional in AllocaInst::getAllocationSizeInBits.
  • Add comments describing the added test cases.
Jun 25 2018, 9:32 AM
bjope created D48547: Improve ConvertDebugDeclareToDebugValue.
Jun 25 2018, 6:59 AM
bjope added a comment to D48323: Derive GEP index type from Data Layout (cont).

AFAIK BasicAA assumes that all GEP indices have a common type. So "normalization" is for example needed before passes like GVN that uses MemoryDependenceResults, that is using BasicAA. Or maybe that is a bug in BasicAA?

Passes like InstCombine might skip doing rewrites in some basic blocks (e.g. if they are unreachable from entry). So I do not think that we can't rely on InstCombine doing a normalization for all GEP:s in the function.
So are we moving towards always using the "normalized" type from scratch (in all passes) when creating a GEP, or what is the plan here?

I don't think that we are going towards the always-normalized GEP, not within these changes, at least. We say that when normalized, the type of index should not be always a pointer type. Each target may specify index type in the Data Layout.
This is about my patches.

I don't know how is the canonization of the GEP form important for the alias analysis. It's more for loop transformations, SCEVs, GEP combining/simplifications, I think

Jun 25 2018, 1:51 AM

Jun 21 2018

bjope accepted D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink.

We all have some suspicions that it might be awkward to move a DBG_VALUE intrinsic across another one for the *same* *variable*. But we haven't seen that happen in this particular situation when having an end-to-end test with C/C++ input. I'm pretty sure such things can happen in other passes (e.g. CodegenPrepare is moving dbg.value without any checks at all), so it is more like a general concern.

Jun 21 2018, 8:41 AM · debug-info

Jun 20 2018

bjope updated subscribers of D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink.

Just pinging this. @bjope has provided great feedback (thanks!), but I am wondering if anyone else thinks this patch is useful, or if I should abandon it in favor of some other solution?

Jun 20 2018, 10:13 AM · debug-info
bjope committed rL335141: [DAG] Don't map a TableId to itself in the ReplacedValues map.
[DAG] Don't map a TableId to itself in the ReplacedValues map
Jun 20 2018, 9:10 AM
bjope closed D48364: [DAG] Don't map a TableId to itself in the ReplacedValues map.
Jun 20 2018, 9:10 AM
bjope added inline comments to D48364: [DAG] Don't map a TableId to itself in the ReplacedValues map.
Jun 20 2018, 6:39 AM
bjope created D48364: [DAG] Don't map a TableId to itself in the ReplacedValues map.
Jun 20 2018, 6:27 AM
bjope added a comment to D48323: Derive GEP index type from Data Layout (cont).

AFAIK BasicAA assumes that all GEP indices have a common type. So "normalization" is for example needed before passes like GVN that uses MemoryDependenceResults, that is using BasicAA. Or maybe that is a bug in BasicAA?

Jun 20 2018, 1:25 AM
Herald updated subscribers of D42123: Derive GEP index type from Data Layout.
Jun 20 2018, 1:07 AM

Jun 19 2018

bjope accepted D48086: [MIRParser] Update a diagnostic message to use the correct register sigil. NFC.

LGTM!

Jun 19 2018, 10:46 AM
bjope committed rL335031: Remove valueCoversEntireFragment asserts in ConvertDebugDeclareToDebugValue.
Remove valueCoversEntireFragment asserts in ConvertDebugDeclareToDebugValue
Jun 19 2018, 1:46 AM

Jun 18 2018

bjope added inline comments to D48277: [DebugInfo] Keep DBG_VALUE undef in LiveDebugVariables.
Jun 18 2018, 10:45 AM

Jun 15 2018

bjope committed rL334830: Re-apply "[DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue".
Re-apply "[DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue"
Jun 15 2018, 6:55 AM

Jun 14 2018

bjope added a reverting commit for rL334704: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue: rL334732: Revert rL334704: "[DebugInfo] Check size of variable in….
Jun 14 2018, 9:12 AM
bjope committed rL334732: Revert rL334704: "[DebugInfo] Check size of variable in….
Revert rL334704: "[DebugInfo] Check size of variable in…
Jun 14 2018, 9:12 AM
bjope added a comment to D48024: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue.

An assertion introduced by this patch is failing on a build bot: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/25013

Please take a look.

Jun 14 2018, 9:07 AM · debug-info
bjope committed rL334704: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue.
[DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue
Jun 14 2018, 4:28 AM
bjope closed D48024: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue.
Jun 14 2018, 4:28 AM · debug-info

Jun 13 2018

bjope added inline comments to D48024: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue.
Jun 13 2018, 5:48 AM · debug-info
bjope updated the diff for D48024: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue.

After some more testing I discovered a case when the added assert in

void llvm::ConvertDebugDeclareToDebugValue(DbgInfoIntrinsic *DII,
                                           PHINode *APN, DIBuilder &Builder)

failed.

Jun 13 2018, 5:43 AM · debug-info
bjope added a comment to D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink.

@bjope , Thanks for your review and example! I agree that any changes to improve performSink/collectDebugValues should probably be in another patch.

I agree, after spending some time with your hypothetical case, I can imagine there are some other cases that might result in old/intermediate values in the user's variable, at least for a short period of time.

...
renamable $eax = COPY $edi
DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !16
$esi = ADD $edi, 7
DBG_VALUE debug-use $esi, debug-use $noreg, !14, !DIExpression(), debug-location !17
...

Excuse me if I am getting this wrong, the following is my interpretation of your example. Let's assume this example lives in bb0. In the example, sticking with x86_64, the ADD (or ADD32ri) instruction expects tied physical registers, something like $edi = ADD32ri $edi, 7 However, that case would prevent the COPY from sinking, I'm guessing due to a register def. If we were to make this $esi = ADD32ri $esi, 7, that would allow for the COPY to be sunk. Variable !14 would truly be $esi +7 at that program point in bb0, because the DBG_VALUE you introduced will associate $esi as the value for !14. The sunken copy, presumably in bb1, would be the value copied from $edi, and probably the value that a user would expect. Single stepping from the ADD to the sunken COPY would show !14 as being an intermediate or 'old' value until the sunken copy is reached. This doesn't seem wrong to me. It just could present unexpected values to the user, when inspecting !14 between the ADD and sunken COPY. Perhaps, if we were to delete just the DBG_VALUE associated to the ADD in bb0 , perhaps that would encourage a debugger to present 'optimized-out' in this case. I agree with you, any refinements to performSink or collectDebugValues should probably be for another patch. Thanks again for your review.

Jun 13 2018, 12:11 AM · debug-info

Jun 12 2018

bjope added a comment to D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink.

Basically looks good to me now, as I think that getting rid of the debug-invariant problem is important.
When it comes to the sinking of DBG_VALUE instructions I believe that will be as good as the pre-RA version of MachineSinking. I think that could be enough to land this (as getting rid of the debug-invariance probably is more important).

Jun 12 2018, 6:07 AM · debug-info
bjope updated the diff for D48024: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue.

More updates after review.

Jun 12 2018, 5:13 AM · debug-info

Jun 11 2018

bjope updated the diff for D48024: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue.

Additional cleanups.

Jun 11 2018, 10:02 AM · debug-info
bjope updated the diff for D48024: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue.

Updated after comments from aprantl.

Jun 11 2018, 9:59 AM · debug-info
bjope added inline comments to D48024: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue.
Jun 11 2018, 9:03 AM · debug-info
bjope created D48024: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue.
Jun 11 2018, 6:55 AM · debug-info
bjope added a comment to D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink.

This comment in the description does not make sense to me:

Jun 11 2018, 2:45 AM · debug-info

May 30 2018

bjope added inline comments to D47531: [ValueTracking] Fix endless recursion in isKnownNonZero().
May 30 2018, 6:55 AM

May 27 2018

bjope added a watcher for debug-info: bjope.
May 27 2018, 2:42 AM

May 22 2018

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.

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