llvm dev - May 2015 - [LLVMdev] [ScalarEvolution] code on nsw inconsistent with comment

+llvmdev

On Mon, May 11, 2015 at 11:44 AM, Jingyue Wu <jingyue at google.com>
wrote:
> Hi Andrew and Sanjoy,
>
> I was looking at ScalarEvolution::createNodeForGEP (
> http://llvm.org/docs/doxygen/html/ScalarEvolution_8cpp_source.html#l03702),
> and noticed that the code that applies nsw flags to the resultant SCEV is
> inconsistent with the comment.
>
> While the comment says
>
> 03709   // Don't blindly transfer the inbounds flag from the GEP
> instruction to the
> 03710   // Add expression, because the Instruction may be guarded by
> control flow
> 03711   // and the no-overflow bits may not be valid for the expression in
> any
> 03712   // context.
> 03713   SCEV::NoWrapFlags Wrap = GEP->isInBounds() ? SCEV::FlagNSW :
> SCEV::FlagAnyWrap;
>
> the code still transfers nsw to LocalOffset and BaseS+TotalOffset. Should
> the code then be fixed?
>
> Jingyue
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150511/f058c6c6/attachment.html>

The code looks incorrect, AFAICT.  Can you write a test case where
this leads to a miscompile, just to confirm?  I suspect this will show
up in something like

for (...) {
  if (cond)
    t = inbounds_gep(A, idx)
  m = gep(A, idx)
  v = m s< A
}

SCEV should incorrectly fold v to false.

On Mon, May 11, 2015 at 1:02 PM, Jingyue Wu <jingyue at google.com>
wrote:
> +llvmdev
>
> On Mon, May 11, 2015 at 11:44 AM, Jingyue Wu <jingyue at google.com>
wrote:
>>
>> Hi Andrew and Sanjoy,
>>
>> I was looking at ScalarEvolution::createNodeForGEP
>>
(http://llvm.org/docs/doxygen/html/ScalarEvolution_8cpp_source.html#l03702),
>> and noticed that the code that applies nsw flags to the resultant SCEV
is
>> inconsistent with the comment.
>>
>> While the comment says
>>
>> 03709   // Don't blindly transfer the inbounds flag from the GEP
>> instruction to the
>> 03710   // Add expression, because the Instruction may be guarded by
>> control flow
>> 03711   // and the no-overflow bits may not be valid for the expression
in
>> any
>> 03712   // context.
>> 03713   SCEV::NoWrapFlags Wrap = GEP->isInBounds() ? SCEV::FlagNSW :
>> SCEV::FlagAnyWrap;
>>
>> the code still transfers nsw to LocalOffset and BaseS+TotalOffset.
Should
>> the code then be fixed?
>>
>> Jingyue
>
>