[PATCH] D19564: Update Debug Intrinsics in RewriteUsesOfClonedInstructions in LoopRotation

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 27 16:48:12 PDT 2016 

On Wed, Apr 27, 2016 at 4:44 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On Apr 27, 2016, at 3:22 PM, David Blaikie via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>
>
> On Wed, Apr 27, 2016 at 12:46 PM, Thomas Jablin <tjablin at gmail.com> wrote:
>
>> The short answer is, "Yes."
>>
>> If a DbgInfoIntrinsic extended the live range of Value before
>> LoopRotation, then LoopRotation may create a situation where the definition
>> of the Value no dominates the DbgInfoIntrinsic. If so, the proposed patch
>> will insert a PHINode.
>>
>> If DbgInfoIntrinsics can extend the live range of a Value, then register
>> allocation and other aspects of optimization and code generation will
>> inevitably change. The alternative is pruning DbgInfoIntrinsics if they use
>> Values that no longer dominate them and when they would extend the
>> live-range of a Value.
>>
>
> I believe this (alternative) ^ would be necessary. I don't know if the
> right thing to do is prune the intrinsic, or just replace its value with
> undef, etc. I forget/don't know how we usually do it.
>
>
> Let me know if I misunderstand something here, but AFAIK the register
> allocator goes out of its way to treat debug intrinsics as transparent.
>

You might have more context here than I do - it just sounded like the
change meant a use of a value, and a phi, that would otherwise not exist &
that seemed like it could affect the result. I could be wrong here, though.
Just wanted to highlight it & was making my best-guess at what would be
behavior-preserving or not.

Totally up for other people to chime in here, I don't feel sufficiently
informed to do final review on this change anyway. Was just trying to cover
some parts that might've needed addressing regardless.

- Dave


>
> -- adrian
>
>
>
>>
>> If the DbgInfoIntrinsic is inside the live-range of a Value and does not
>> extend it, no additional PHINodes will be need to be inserted.
>>
>> On Wed, Apr 27, 2016 at 1:27 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>> Does this cause changes in non-debug IR in the presence of debug info
>>> intrinsics? (you mention that "If there were uses of values produced by
>>> these instructions that were outside the loop, we have to insert PHI nodes
>>> to merge the two values." - that sounds like it could be a problem, as
>>> enabling debug info is not intended to change optimization/code generation)
>>>
>>> On Tue, Apr 26, 2016 at 3:34 PM, Thomas Jablin via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> tjablin created this revision.
>>>> tjablin added reviewers: kbarton, hfinkel, cycheng.
>>>> tjablin added a subscriber: llvm-commits.
>>>> Herald added a subscriber: mzolotukhin.
>>>>
>>>> Loop rotation clones instruction from the old header into the
>>>> preheader. If there were uses of values produced by these instructions that
>>>> were outside the loop, we have to insert PHI nodes to merge the two values.
>>>> If the values are used by DbgIntrinsics they will be used as a
>>>> MetadataAsValue of a ValueAsMetadata of the original values, and iterating
>>>> all of the uses of the original value will not update the DbgIntrinsics.
>>>> The new code checks if the values are used by DbgIntrinsics and if so,
>>>> updates them using essentially the same logic as the original code.
>>>>
>>>> The attached testcase demonstrates the issue. Without the fix, the
>>>> DbgIntrinic outside the loop uses values computed inside the loop, even
>>>> though these values do not dominate the DbgIntrinsic.
>>>>
>>>> http://reviews.llvm.org/D19564
>>>>
>>>> Files:
>>>>   lib/Transforms/Scalar/LoopRotation.cpp
>>>>   test/Transforms/LoopRotate/dbgvalue.ll
>>>>
>>>> Index: test/Transforms/LoopRotate/dbgvalue.ll
>>>> ===================================================================
>>>> --- test/Transforms/LoopRotate/dbgvalue.ll
>>>> +++ test/Transforms/LoopRotate/dbgvalue.ll
>>>> @@ -7,6 +7,7 @@
>>>>  ; CHECK-LABEL: define i32 @tak(
>>>>  ; CHECK: entry
>>>>  ; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %x
>>>> +; CHECK: tail call void @llvm.dbg.value(metadata i32 %call
>>>>
>>>>  entry:
>>>>    br label %tailrecurse
>>>> @@ -37,6 +38,44 @@
>>>>    ret i32 %z.tr, !dbg !17
>>>>  }
>>>>
>>>> +define i32 @tak2(i32 %x, i32 %y, i32 %z) nounwind ssp !dbg !0 {
>>>> +; CHECK-LABEL: define i32 @tak2(
>>>> +; CHECK: entry
>>>> +; CHECK: tail call void @llvm.dbg.value(metadata i32 %x.tr
>>>> +; CHECK: tail call void @llvm.dbg.value(metadata i32 %x.tr
>>>> +
>>>> +entry:
>>>> +  br label %tailrecurse
>>>> +
>>>> +tailrecurse:                                      ; preds = %if.then,
>>>> %entry
>>>> +  %x.tr = phi i32 [ %x, %entry ], [ %call, %if.then ]
>>>> +  %y.tr = phi i32 [ %y, %entry ], [ %call9, %if.then ]
>>>> +  %z.tr = phi i32 [ %z, %entry ], [ %call14, %if.then ]
>>>> +  %cmp = icmp slt i32 %y.tr, %x.tr, !dbg !12
>>>> +  br i1 %cmp, label %if.then, label %if.end, !dbg !12
>>>> +
>>>> +if.then:                                          ; preds =
>>>> %tailrecurse
>>>> +  tail call void @llvm.dbg.value(metadata i32 %x.tr, i64 0, metadata
>>>> !6, metadata !DIExpression()), !dbg !7
>>>> +  tail call void @llvm.dbg.value(metadata i32 %y.tr, i64 0, metadata
>>>> !8, metadata !DIExpression()), !dbg !9
>>>> +  tail call void @llvm.dbg.value(metadata i32 %z.tr, i64 0, metadata
>>>> !10, metadata !DIExpression()), !dbg !11
>>>> +  %sub = sub nsw i32 %x.tr, 1, !dbg !14
>>>> +  %call = tail call i32 @tak(i32 %sub, i32 %y.tr, i32 %z.tr), !dbg !14
>>>> +  %sub6 = sub nsw i32 %y.tr, 1, !dbg !14
>>>> +  %call9 = tail call i32 @tak(i32 %sub6, i32 %z.tr, i32 %x.tr), !dbg
>>>> !14
>>>> +  %sub11 = sub nsw i32 %z.tr, 1, !dbg !14
>>>> +  %call14 = tail call i32 @tak(i32 %sub11, i32 %x.tr, i32 %y.tr),
>>>> !dbg !14
>>>> +  br label %tailrecurse
>>>> +
>>>> +if.end:                                           ; preds =
>>>> %tailrecurse
>>>> +  tail call void @llvm.dbg.value(metadata i32 %x.tr, i64 0, metadata
>>>> !6, metadata !DIExpression()), !dbg !7
>>>> +  tail call void @llvm.dbg.value(metadata i32 %y.tr, i64 0, metadata
>>>> !8, metadata !DIExpression()), !dbg !9
>>>> +  tail call void @llvm.dbg.value(metadata i32 %z.tr, i64 0, metadata
>>>> !10, metadata !DIExpression()), !dbg !11
>>>> +  br label %return, !dbg !16
>>>> +
>>>> +return:                                           ; preds = %if.end
>>>> +  ret i32 %z.tr, !dbg !17
>>>> +}
>>>> +
>>>>  @channelColumns = external global i64
>>>>  @horzPlane = external global i8*, align 8
>>>>
>>>> Index: lib/Transforms/Scalar/LoopRotation.cpp
>>>> ===================================================================
>>>> --- lib/Transforms/Scalar/LoopRotation.cpp
>>>> +++ lib/Transforms/Scalar/LoopRotation.cpp
>>>> @@ -108,6 +108,33 @@
>>>>
>>>>        // Anything else can be handled by SSAUpdater.
>>>>        SSA.RewriteUse(U);
>>>> +
>>>> +      // Replace MetadataAsValue(ValueAsMetadata(OrigHeaderVal)) uses
>>>> in debug
>>>> +      // intrinsics
>>>> +      LLVMContext &C = OrigHeader->getContext();
>>>> +      if (ValueAsMetadata *VAM =
>>>> ValueAsMetadata::getIfExists(OrigHeaderVal)) {
>>>> +        if (MetadataAsValue *MAV = MetadataAsValue::getIfExists(C,
>>>> VAM)) {
>>>> +          for (auto UI = MAV->use_begin(), E = MAV->use_end(); UI !=
>>>> E; ) {
>>>> +            // Grab the use before incrementing the iterator.
>>>> +            Use &U = *UI++;
>>>> +            DbgInfoIntrinsic *UserInst =
>>>> cast<DbgInfoIntrinsic>(U.getUser());
>>>> +            BasicBlock *UserBB = UserInst->getParent();
>>>> +
>>>> +            // The original users in the OrigHeader are already using
>>>> the
>>>> +            // original definitions.
>>>> +            if (UserBB == OrigHeader)
>>>> +              continue;
>>>> +
>>>> +            // Users in the OrigPreHeader need to use the value to
>>>> which the
>>>> +            // original definitions are mapped and anything else can
>>>> be handled
>>>> +            // by the SSAUpdater
>>>> +            Value *NewVal = UserBB == OrigPreheader ?
>>>> +              OrigPreHeaderVal :
>>>> +              SSA.GetValueInMiddleOfBlock(UserBB);
>>>> +            U = MetadataAsValue::get(C, ValueAsMetadata::get(NewVal));
>>>> +          }
>>>> +        }
>>>> +      }
>>>>      }
>>>>    }
>>>>  }
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>
>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160427/9dd4cc51/attachment.html>

More information about the llvm-commits mailing list