[PATCH] Fix for incorrect address sinking in the presence of potential overflows

Louis Gerbarg lgg at apple.com
Mon Mar 24 13:34:48 PDT 2014


Okay. here is a patch with revised comments.

Louis

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-for-incorrect-address-sinking-in-the-presence-of.patch
Type: application/octet-stream
Size: 2839 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140324/1af16033/attachment.obj>
-------------- next part --------------


On Mar 24, 2014, at 10:17 AM, Jim Grosbach <grosbach at apple.com> wrote:

> Hi Louis,
> 
> The patch looks good to me and conservatively correct. A few minor notes inline below:
> 
>> From 19a985efd276b9cdb4164f9a4478dedeb3e0cb6e Mon Sep 17 00:00:00 2001
>> From: Louis Gerbarg <lgg at apple.com>
>> Date: Fri, 21 Mar 2014 14:14:42 -0700
>> Subject: [PATCH] Fix for incorrect address sinking in the presence of
>> potential overflows
>> 
>> ---
>> lib/CodeGen/CodeGenPrepare.cpp   |  5 ++++-
>> test/CodeGen/X86/sunkaddr-ext.ll | 22 ++++++++++++++++++++++
>> 2 files changed, 26 insertions(+), 1 deletion(-)
>> create mode 100644 test/CodeGen/X86/sunkaddr-ext.ll
>> 
>> diff --git a/lib/CodeGen/CodeGenPrepare.cpp b/lib/CodeGen/CodeGenPrepare.cpp
>> index 428d8af..e1c0fc6 100644
>> --- a/lib/CodeGen/CodeGenPrepare.cpp
>> +++ b/lib/CodeGen/CodeGenPrepare.cpp
>> @@ -2438,7 +2438,10 @@ bool CodeGenPrepare::OptimizeMemoryInst(Instruction *MemoryInst, Value *Addr,
>>                  cast<IntegerType>(V->getType())->getBitWidth()) {
>>         V = Builder.CreateTrunc(V, IntPtrTy, "sunkaddr");
>>       } else {
>> -        V = Builder.CreateSExt(V, IntPtrTy, "sunkaddr");
>> +        // Bail out gracefully if widths do not match
> 
> Missing terminating period on the comment. Probably also worth going into a bit more detail on why bailing out is the right decision here to prevent some clever soul later on thinking “oh, I can just [sz]ext here and things will be better!” and reintroducing the problem. Nothing grandiose, just a mention that we can’t change the bit width because we don’t know if the expression relies on wrapping or not.
> 
>> +        if (Result != AddrMode.BaseReg)
>> +            cast<Instruction>(Result)->eraseFromParent();
>> +        return false;
>>       }
>>       if (AddrMode.Scale != 1)
>>         V = Builder.CreateMul(V, ConstantInt::get(IntPtrTy, AddrMode.Scale),
>> diff --git a/test/CodeGen/X86/sunkaddr-ext.ll b/test/CodeGen/X86/sunkaddr-ext.ll
>> new file mode 100644
>> index 0000000..9d4a629
>> --- /dev/null
>> +++ b/test/CodeGen/X86/sunkaddr-ext.ll
>> @@ -0,0 +1,22 @@
>> +; RUN: llc < %s | FileCheck %s
>> +
> 
> Likewise, a comment here explaining what this is testing for and why would be good. I’ve spent way more time than I care to talk about investigating test failures trying to figure out whether the change to the expected output is innocuous or not because I don’t know, and it’s not obvious from the test or commit history, what the purpose of the test is.
> 
>> +target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
>> +target triple = "x86_64-apple-macosx10.9.0"
>> +
>> +; Function Attrs: nounwind ssp uwtable
>> +define void @test_sink(i8* %arg1, i32 %arg2, i8 %arg3) #0 {
>> +  %tmp1 = add i32 -2147483648, %arg2
>> +  %tmp2 = add i32 -2147483648, %tmp1
>> +  %tmp3 = getelementptr i8* %arg1, i32 %arg2
>> +  br label %bb1
>> +
>> +bb1:
>> +  %tmp4 = getelementptr i8* %arg1, i32 %tmp2
>> +  store i8 %arg3, i8* %tmp4
>> +  ret void;
>> +}
>> +
>> +; CHECK-LABEL: test_sink:
>> +; CHECK:   movslq  %esi, [[TEMP:%[a-z0-9]+]]
>> +; CHECK:   movb    %dl, (%rdi,[[TEMP]])
>> +; CHECK:   retq
>> -- 
> On Mar 21, 2014, at 4:24 PM, Louis Gerbarg <lgg at apple.com> wrote:
> 
>> The attached patch is a fix for errors that can occur when sinking an address from another basic block. Essentially what happens is that sunkaddr is a manually lowered GEP, but that when using an integer whose width is less than a pointer an overflow can happen that leads to invalid address generation. Technically one can still legally do the operation on constrained ranges or if the add has nsw/nuw annotations, but as a practical matter this is an edge case that almost never occurs and the data necessary to figure out if the transform is legal is not available at the point where it needs to be made, so in the cases where the address generation can be incorrect I am just backing out the optimization.
>> 
>> The included regression test will generate malformed code when run on trunk. I am attaching samples of both the correct and the malformed code.
>> 
>> The performance impact of this should be essentially non-existent, this type of address sinking with type expansion happens only once or twice in the entirety of the regression tests and LNT.
>> 
>> Louis
>> 
>> <0001-Fix-for-incorrect-address-sinking-in-the-presence-of.patch><good.s><bad.s>_______________________________________________
>> 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