[PATCH] Disable tail merge of call sites when building with any sanitizer
Evgeniy Stepanov
eugeni.stepanov at gmail.com
Tue Nov 26 03:25:32 PST 2013
Btw, is this an issue for sampling-based PGO as well? I guess
optimizations like this one could affect the estimated branch
probabilities.
On Tue, Nov 19, 2013 at 1:29 AM, David Blaikie <dblaikie at gmail.com> wrote:
> Same as:
>
> Do we have precedence for this kind of change (where sanitizers affect
> optimizations in arbitrary internal ways - not simply by
> enabling/disabling
> certain passes)? If not, does this need some deeper discussion about
> alternatives (is it important that we be able to produce equivalent code
> without the sanitizers enabled?)?
>
> http://llvm-reviews.chandlerc.com/D2214
>
>
> On Mon, Nov 18, 2013 at 7:31 AM, Evgeniy Stepanov <eugenis at google.com>
> wrote:
>>
>> Merging call sites leads to very poor and misleading stack traces. They
>> have wrong line numbers for some of the frames, but in a very non-obvious
>> way, which makes reasoning about such reports very difficult.
>>
>>
>> http://llvm-reviews.chandlerc.com/D2215
>>
>> Files:
>> lib/CodeGen/BranchFolding.h
>> lib/CodeGen/BranchFolding.cpp
>> test/CodeGen/X86/tail-merge-msan.ll
>>
>> Index: lib/CodeGen/BranchFolding.h
>> ===================================================================
>> --- lib/CodeGen/BranchFolding.h
>> +++ lib/CodeGen/BranchFolding.h
>> @@ -86,6 +86,7 @@
>> std::vector<SameTailElt> SameTails;
>>
>> bool EnableTailMerge;
>> + bool EnableTailMergeCalls;
>> bool EnableHoistCommonCode;
>> const TargetInstrInfo *TII;
>> const TargetRegisterInfo *TRI;
>> Index: lib/CodeGen/BranchFolding.cpp
>> ===================================================================
>> --- lib/CodeGen/BranchFolding.cpp
>> +++ lib/CodeGen/BranchFolding.cpp
>> @@ -90,7 +90,6 @@
>>
>> getAnalysisIfAvailable<MachineModuleInfo>());
>> }
>>
>> -
>> BranchFolder::BranchFolder(bool defaultEnableTailMerge, bool CommonHoist)
>> {
>> switch (FlagEnableTailMerge) {
>> case cl::BOU_UNSET: EnableTailMerge = defaultEnableTailMerge; break;
>> @@ -169,7 +168,7 @@
>> return true;
>> }
>>
>> -/// OptimizeFunction - Perhaps branch folding, tail merging and other
>> +/// OptimizeFunction - Perform branch folding, tail merging and other
>> /// CFG optimizations on the given function.
>> bool BranchFolder::OptimizeFunction(MachineFunction &MF,
>> const TargetInstrInfo *tii,
>> @@ -184,6 +183,13 @@
>> MMI = mmi;
>> RS = NULL;
>>
>> + // Tail merging code that contains function calls breaks symbolization
>> of
>> + // stack traces.
>> + EnableTailMergeCalls =
>> + !MF.getFunction()->hasFnAttribute(Attribute::SanitizeAddress) &&
>> + !MF.getFunction()->hasFnAttribute(Attribute::SanitizeMemory) &&
>> + !MF.getFunction()->hasFnAttribute(Attribute::SanitizeThread);
>> +
>> // Use a RegScavenger to help update liveness when required.
>> MachineRegisterInfo &MRI = MF.getRegInfo();
>> if (MRI.tracksLiveness() && TRI->trackLivenessAfterRegAlloc(MF))
>> @@ -305,7 +311,8 @@
>> static unsigned ComputeCommonTailLength(MachineBasicBlock *MBB1,
>> MachineBasicBlock *MBB2,
>> MachineBasicBlock::iterator &I1,
>> - MachineBasicBlock::iterator &I2)
>> {
>> + MachineBasicBlock::iterator &I2,
>> + bool EnableTailMergeCalls) {
>> I1 = MBB1->end();
>> I2 = MBB2->end();
>>
>> @@ -337,7 +344,7 @@
>> --I2;
>> }
>> // I1, I2==first (untested) non-DBGs preceding known match
>> - if (!I1->isIdenticalTo(I2) ||
>> + if (!I1->isIdenticalTo(I2) || (!EnableTailMergeCalls && I1->isCall())
>> ||
>> // FIXME: This check is dubious. It's used to get around a
>> problem where
>> // people incorrectly expect inline asm directives to remain in
>> the same
>> // relative order. This is untenable because normal compiler
>> @@ -519,15 +526,16 @@
>> /// and decide if it would be profitable to merge those tails. Return
>> the
>> /// length of the common tail and iterators to the first common
>> instruction
>> /// in each block.
>> -static bool ProfitableToMerge(MachineBasicBlock *MBB1,
>> - MachineBasicBlock *MBB2,
>> +static bool ProfitableToMerge(MachineBasicBlock *MBB1, MachineBasicBlock
>> *MBB2,
>> unsigned minCommonTailLength,
>> unsigned &CommonTailLen,
>> MachineBasicBlock::iterator &I1,
>> MachineBasicBlock::iterator &I2,
>> MachineBasicBlock *SuccBB,
>> - MachineBasicBlock *PredBB) {
>> - CommonTailLen = ComputeCommonTailLength(MBB1, MBB2, I1, I2);
>> + MachineBasicBlock *PredBB,
>> + bool EnableTailMergeCalls) {
>> + CommonTailLen =
>> + ComputeCommonTailLength(MBB1, MBB2, I1, I2, EnableTailMergeCalls);
>> if (CommonTailLen == 0)
>> return false;
>> DEBUG(dbgs() << "Common tail length of BB#" << MBB1->getNumber()
>> @@ -604,9 +612,8 @@
>> for (MPIterator I = prior(CurMPIter); I->getHash() == CurHash ; --I)
>> {
>> unsigned CommonTailLen;
>> if (ProfitableToMerge(CurMPIter->getBlock(), I->getBlock(),
>> - minCommonTailLength,
>> - CommonTailLen, TrialBBI1, TrialBBI2,
>> - SuccBB, PredBB)) {
>> + minCommonTailLength, CommonTailLen,
>> TrialBBI1,
>> + TrialBBI2, SuccBB, PredBB,
>> EnableTailMergeCalls)) {
>> if (CommonTailLen > maxCommonTailLength) {
>> SameTails.clear();
>> maxCommonTailLength = CommonTailLen;
>> Index: test/CodeGen/X86/tail-merge-msan.ll
>> ===================================================================
>> --- test/CodeGen/X86/tail-merge-msan.ll
>> +++ test/CodeGen/X86/tail-merge-msan.ll
>> @@ -0,0 +1,80 @@
>> +; RUN: llc < %s -march=x86-64 -mtriple=x86_64-unknown-linux-gnu
>> -asm-verbose=false | FileCheck %s
>> +
>> +declare void @callee()
>> +
>> +; Test that call sites are not tail merged with sanitize_address
>> +
>> +define i32 @caller1(i32 %x) sanitize_address {
>> +; CHECK: caller1:
>> +; CHECK: callq callee
>> +; CHECK: callq callee
>> +; CHECK: ret
>> +entry:
>> + switch i32 %x, label %if.end3 [
>> + i32 1, label %if.then
>> + i32 2, label %if.then2
>> + ]
>> +
>> +if.then: ; preds = %entry
>> + tail call void @callee()
>> + br label %if.end3
>> +
>> +if.then2: ; preds = %entry
>> + tail call void @callee()
>> + br label %if.end3
>> +
>> +if.end3: ; preds = %entry,
>> %if.then2, %if.then
>> + ret i32 0
>> +}
>> +
>> +
>> +; Test that call sites are not tail merged with sanitize_memory
>> +
>> +define i32 @caller2(i32 %x) sanitize_memory {
>> +; CHECK: caller2:
>> +; CHECK: callq callee
>> +; CHECK: callq callee
>> +; CHECK: ret
>> +entry:
>> + switch i32 %x, label %if.end3 [
>> + i32 1, label %if.then
>> + i32 2, label %if.then2
>> + ]
>> +
>> +if.then: ; preds = %entry
>> + tail call void @callee()
>> + br label %if.end3
>> +
>> +if.then2: ; preds = %entry
>> + tail call void @callee()
>> + br label %if.end3
>> +
>> +if.end3: ; preds = %entry,
>> %if.then2, %if.then
>> + ret i32 0
>> +}
>> +
>> +
>> +; Test that call sites are not tail merged with sanitize_thread
>> +
>> +define i32 @caller3(i32 %x) sanitize_thread {
>> +; CHECK: caller3:
>> +; CHECK: callq callee
>> +; CHECK: callq callee
>> +; CHECK: ret
>> +entry:
>> + switch i32 %x, label %if.end3 [
>> + i32 1, label %if.then
>> + i32 2, label %if.then2
>> + ]
>> +
>> +if.then: ; preds = %entry
>> + tail call void @callee()
>> + br label %if.end3
>> +
>> +if.then2: ; preds = %entry
>> + tail call void @callee()
>> + br label %if.end3
>> +
>> +if.end3: ; preds = %entry,
>> %if.then2, %if.then
>> + ret i32 0
>> +}
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list