<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 17, 2014 at 5:17 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Sanjay,<div><br></div><div>I do not think this change is correct.</div><div>I have attached a test case with (before.ll) and without (after.ll) that change and the related C file (test.c).</div></div></blockquote><div><br></div><div><span style="color:rgb(0,0,0);font-family:'Courier New',Courier,monospace;font-size:14px;white-space:pre-wrap">signed short a = 8*8*8*548.54 - 3.14; // <-- the value of a is undef.</span><br></div><div><span style="color:rgb(0,0,0);font-family:'Courier New',Courier,monospace;font-size:14px;white-space:pre-wrap"><br></span></div>This line has undefined behavior; both C and C++ are very clear about this. The transformation is correct at the language semantics level. And it appears correct at the IR level too, whether you interpret "the results are undefined" as meaning undefined behavior or (strictly weaker) an undef value. It would be correct to treat this as poison, not just as undef, from a language semantics perspective.<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div>To reproduce, compile with O1 or more and run the produced executable. The correct behavior is do not assert :).</div><div><br></div><div>Please revert or fix it.</div><div><br><div><span class=""><blockquote type="cite"><div>On Oct 10, 2014, at 4:00 PM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:</div><br><div>Author: spatel<br>Date: Fri Oct 10 18:00:21 2014<br>New Revision: 219542<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=219542&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=219542&view=rev</a><br>Log:<br>Return undef on FP <-> Int conversions that overflow (PR21330).<br><br>The LLVM Lang Ref states for signed/unsigned int to float conversions:<br>"If the value cannot fit in the floating point value, the results are undefined."<br><br>And for FP to signed/unsigned int:<br>"If the value cannot fit in ty2, the results are undefined."<br><br>This matches the C definitions.<br></div></blockquote><div><br></div></span><div>Yes, this matches the C definitions, but the rules of propagating the undef value are not strictly the same in C and LLVM IR AFAICT.</div><div>Here is an example:</div><div>In (pseudo) C,</div><div>a = undef // a is undef;</div><div>if (something)</div><div> b = val;</div><div>else</div><div> b = a; // b is undef, but it should be the same value as a.</div><div>assert (something || b == a);</div><div><br></div><div>The related (pseudo) IR would be:</div><div>a = undef</div><div>b = select i1 something, val, a</div><div>cmpVal = cmp eq b, a</div><div>assertVal = or something, cmpVal</div><div>call assert(assertVal)</div><div><br></div><div>Now, the actual IR will be, because of constant propagation:</div><div><div>b = select i1 something, val, undef // <— we lose the information that the next two undef are the same value.</div><div>cmpVal = cmp eq b, undef</div><div>assertVal = or something, cmpVal</div><div>call assert(assertVal)</div><div><br></div><div>This is still fine as long as undef are expanded into the same constant.</div><div>However, the LangRef states that the following transformation is valid:</div><div><pre style="font-family:Consolas,'Deja Vu Sans Mono','Bitstream Vera Sans Mono',monospace;font-size:0.95em;line-height:15.960000038147px;padding:0.5em;border:1px solid rgb(204,204,204);background-color:rgb(248,248,248)"> <span style="color:rgb(187,96,213)">%C</span> <span>=</span> <span style="color:rgb(0,112,32);font-weight:bold">select</span> <span style="color:rgb(187,96,213)">%X</span><span>,</span> <span style="color:rgb(187,96,213)">%Y</span><span>,</span> <span style="color:rgb(0,112,32);font-weight:bold">undef</span>
<span style="color:rgb(0,32,112);font-weight:bold">Safe:</span>
<span style="color:rgb(187,96,213)">%C</span> <span>=</span> <span style="color:rgb(187,96,213)">%Y</span>
<span style="color:rgb(0,32,112);font-weight:bold">Unsafe:</span>
<span style="color:rgb(187,96,213)">%C</span> <span>=</span> <span style="color:rgb(0,112,32);font-weight:bold">undef</span></pre><div>Therefore, we end up with the following IR:</div></div><div><div>b = val // <— now, the value of b may not be a when ‘something' is false, which is incorrect. </div><div>cmpVal = cmp eq b, undef</div><div>assertVal = or something, cmpVal</div><div>call assert(assertVal)</div></div><div><br></div><div>Thanks,</div><div>-Quentin</div><div></div></div></div></div></div><br><div style="word-wrap:break-word"><div><div><div><div></div></div></div></div></div><br><div style="word-wrap:break-word"><div><div><div><div></div></div></div></div></div><br><div style="word-wrap:break-word"><div><div><div><div></div></div><div><br></div><blockquote type="cite"><div><br>The existing behavior pins to infinity or a max int value, but that may just<br>lead to more confusion as seen in:<br><a href="http://llvm.org/bugs/show_bug.cgi?id=21130" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=21130</a><br><br>Returning undef will hopefully lead to a less silent failure.<br><br>Differential Revision: <a href="http://reviews.llvm.org/D5603" target="_blank">http://reviews.llvm.org/D5603</a><br><br><br>Modified:<br> llvm/trunk/lib/IR/ConstantFold.cpp<br> llvm/trunk/test/Transforms/InstCombine/cast.ll<br><br>Modified: llvm/trunk/lib/IR/ConstantFold.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/ConstantFold.cpp?rev=219542&r1=219541&r2=219542&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/ConstantFold.cpp?rev=219542&r1=219541&r2=219542&view=diff</a><br>==============================================================================<br>--- llvm/trunk/lib/IR/ConstantFold.cpp (original)<br>+++ llvm/trunk/lib/IR/ConstantFold.cpp Fri Oct 10 18:00:21 2014<br>@@ -593,8 +593,13 @@ Constant *llvm::ConstantFoldCastInstruct<br> bool ignored;<br> uint64_t x[2]; <br> uint32_t DestBitWidth = cast<IntegerType>(DestTy)->getBitWidth();<br>- (void) V.convertToInteger(x, DestBitWidth, opc==Instruction::FPToSI,<br>- APFloat::rmTowardZero, &ignored);<br>+ if (APFloat::opInvalidOp ==<br>+ V.convertToInteger(x, DestBitWidth, opc==Instruction::FPToSI,<br>+ APFloat::rmTowardZero, &ignored)) {<br>+ // Undefined behavior invoked - the destination type can't represent<br>+ // the input constant.<br>+ return UndefValue::get(DestTy);<br>+ }<br> APInt Val(DestBitWidth, x);<br> return ConstantInt::get(FPC->getContext(), Val);<br> }<br>@@ -653,9 +658,13 @@ Constant *llvm::ConstantFoldCastInstruct<br> APInt api = CI->getValue();<br> APFloat apf(DestTy->getFltSemantics(),<br> APInt::getNullValue(DestTy->getPrimitiveSizeInBits()));<br>- (void)apf.convertFromAPInt(api, <br>- opc==Instruction::SIToFP,<br>- APFloat::rmNearestTiesToEven);<br>+ if (APFloat::opOverflow &<br>+ apf.convertFromAPInt(api, opc==Instruction::SIToFP,<br>+ APFloat::rmNearestTiesToEven)) {<br>+ // Undefined behavior invoked - the destination type can't represent<br>+ // the input constant.<br>+ return UndefValue::get(DestTy);<br>+ }<br> return ConstantFP::get(V->getContext(), apf);<br> }<br> return nullptr;<br><br>Modified: llvm/trunk/test/Transforms/InstCombine/cast.ll<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/cast.ll?rev=219542&r1=219541&r2=219542&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/cast.ll?rev=219542&r1=219541&r2=219542&view=diff</a><br>==============================================================================<br>--- llvm/trunk/test/Transforms/InstCombine/cast.ll (original)<br>+++ llvm/trunk/test/Transforms/InstCombine/cast.ll Fri Oct 10 18:00:21 2014<br>@@ -1050,3 +1050,37 @@ define i8 @test85(i32 %a) {<br> ; CHECK: [[SHR:%.*]] = lshr exact i32 [[ADD]], 23<br> ; CHECK: [[CST:%.*]] = trunc i32 [[SHR]] to i8<br> }<br>+<br>+; Overflow on a float to int or int to float conversion is undefined (PR21130).<br>+<br>+define i8 @overflow_fptosi() {<br>+ %i = fptosi double 1.56e+02 to i8<br>+ ret i8 %i<br>+; CHECK-LABEL: @overflow_fptosi(<br>+; CHECK-NEXT: ret i8 undef <br>+}<br>+<br>+define i8 @overflow_fptoui() {<br>+ %i = fptoui double 2.56e+02 to i8<br>+ ret i8 %i<br>+; CHECK-LABEL: @overflow_fptoui(<br>+; CHECK-NEXT: ret i8 undef <br>+}<br>+<br>+; The maximum float is approximately 2 ** 128 which is 3.4E38. <br>+; The constant below is 4E38. Use a 130 bit integer to hold that<br>+; number; 129-bits for the value + 1 bit for the sign.<br>+define float @overflow_uitofp() {<br>+ %i = uitofp i130 400000000000000000000000000000000000000 to float<br>+ ret float %i<br>+; CHECK-LABEL: @overflow_uitofp(<br>+; CHECK-NEXT: ret float undef <br>+}<br>+<br>+define float @overflow_sitofp() {<br>+ %i = sitofp i130 400000000000000000000000000000000000000 to float<br>+ ret float %i<br>+; CHECK-LABEL: @overflow_sitofp(<br>+; CHECK-NEXT: ret float undef <br>+}<br>+<br><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></div></blockquote></div><br></div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>