Hi,
Attached is the final version of the patch, adding the requested
FIXME. If this is ok, can
somebody check it in ?
thanks
Zoltan
On Tue, Dec 9, 2008 at 9:58 PM, Bill Wendling <isanbard at gmail.com>
wrote:> On Tue, Dec 9, 2008 at 6:11 AM, Zoltan Varga <vargaz at gmail.com>
wrote:
>> Hi,
>>
>> Here is the next iteration of the patch. The only comment not
>> addressed is this one:
>>
> Thanks! It's looking good.
>
>>> It would be better to implement a target-independent check for
>>> overflow for the "Legal" case (like how SADDO does).
Hacker's > Delight
>>> has some hints on how to do this. It's not easy for the signed
case,
>>> but is do-able.
>>
>> It can be lowered to a division + a branch, so it would be
>> inefficient, plus it would
>> be a lot of work to implement it correctly (for me at least).
>>
> Okay. It would be tricky. Please put a "FIXME" in there
indicating
> that it would be nice to have at some point. It's okay to submit this.
>
> -bw
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-ovf.diff
Type: text/x-diff
Size: 37966 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20081209/cd928d07/attachment.diff>
Applied. Thanks, Zoltan! -bw On Tue, Dec 9, 2008 at 1:12 PM, Zoltan Varga <vargaz at gmail.com> wrote:> Hi, > > Attached is the final version of the patch, adding the requested > FIXME. If this is ok, can > somebody check it in ? > > thanks > > Zoltan > > On Tue, Dec 9, 2008 at 9:58 PM, Bill Wendling <isanbard at gmail.com> wrote: >> On Tue, Dec 9, 2008 at 6:11 AM, Zoltan Varga <vargaz at gmail.com> wrote: >>> Hi, >>> >>> Here is the next iteration of the patch. The only comment not >>> addressed is this one: >>> >> Thanks! It's looking good. >> >>>> It would be better to implement a target-independent check for >>>> overflow for the "Legal" case (like how SADDO does). Hacker's > Delight >>>> has some hints on how to do this. It's not easy for the signed case, >>>> but is do-able. >>> >>> It can be lowered to a division + a branch, so it would be >>> inefficient, plus it would >>> be a lot of work to implement it correctly (for me at least). >>> >> Okay. It would be tricky. Please put a "FIXME" in there indicating >> that it would be nice to have at some point. It's okay to submit this. >> >> -bw >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >
Hi,
The add.with.overflow instrinsics don't seem to work with constant
arguments, i.e.
changing the call in add-with-overflow.ll to:
%t = call {i32, i1} @llvm.sadd.with.overflow.i32(i32 0, i32 0)
causes the following exception when running the codegen tests:
llc: DAGCombiner.cpp:646:
void<unnamed>::DAGCombiner::Run(llvm::CombineLevel): Assertion
`N->getValueType(0) == RV.getValueType() && N->getNumValues() == 1
&&
"Type mismatch"' failed.
0 llc 0x00000000010601ef
1 llc 0x00000000010604ec
2 libc.so.6 0x00007f58a754df60
3 libc.so.6 0x00007f58a754ded5 gsignal + 53
4 libc.so.6 0x00007f58a754f3f3 abort + 387
5 libc.so.6 0x00007f58a7546dc9 __assert_fail + 233
6 llc 0x0000000000cd0444
7 llc 0x0000000000cd0575
llvm::SelectionDAG::Combine(llvm::CombineLevel, llvm::AliasAnalysis&,
bool) + 55
8 llc 0x0000000000d5075a
llvm::SelectionDAGISel::CodeGenAndEmitDAG() + 2176
9 llc 0x0000000000d52a6e
llvm::SelectionDAGISel::SelectBasicBlock(llvm::BasicBlock*,
llvm::ilist_iterator<llvm::Instruction>,
llvm::ilist_iterator<llvm::Instruction>) + 642
10 llc 0x0000000000d533e5
llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function&,
llvm::MachineFunction&, llvm::MachineModuleInfo*,
llvm::TargetInstrInfo const&) + 2175
11 llc 0x0000000000d540f6
llvm::SelectionDAGISel::runOnFunction(llvm::Function&) + 778
12 llc 0x0000000000fedd1d
llvm::FPPassManager::runOnFunction(llvm::Function&) + 239
13 llc 0x0000000000fee860
llvm::FunctionPassManagerImpl::run(llvm::Function&) + 116
14 llc 0x0000000000fee9be
llvm::FunctionPassManager::run(llvm::Function&) + 128
15 llc 0x0000000000822a95 main + 2234
Zoltan
On Tue, Dec 9, 2008 at 11:08 PM, Bill Wendling <isanbard at gmail.com>
wrote:> Applied. Thanks, Zoltan!
>
> -bw
>
> On Tue, Dec 9, 2008 at 1:12 PM, Zoltan Varga <vargaz at gmail.com>
wrote:
>> Hi,
>>
>> Attached is the final version of the patch, adding the requested
>> FIXME. If this is ok, can
>> somebody check it in ?
>>
>> thanks
>>
>> Zoltan
>>
>> On Tue, Dec 9, 2008 at 9:58 PM, Bill Wendling <isanbard at
gmail.com> wrote:
>>> On Tue, Dec 9, 2008 at 6:11 AM, Zoltan Varga <vargaz at
gmail.com> wrote:
>>>> Hi,
>>>>
>>>> Here is the next iteration of the patch. The only comment not
>>>> addressed is this one:
>>>>
>>> Thanks! It's looking good.
>>>
>>>>> It would be better to implement a target-independent check
for
>>>>> overflow for the "Legal" case (like how SADDO
does). Hacker's > Delight
>>>>> has some hints on how to do this. It's not easy for the
signed case,
>>>>> but is do-able.
>>>>
>>>> It can be lowered to a division + a branch, so it would be
>>>> inefficient, plus it would
>>>> be a lot of work to implement it correctly (for me at least).
>>>>
>>> Okay. It would be tricky. Please put a "FIXME" in there
indicating
>>> that it would be nice to have at some point. It's okay to
submit this.
>>>
>>> -bw
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
On Tue, Dec 9, 2008 at 1:12 PM, Zoltan Varga <vargaz at gmail.com> wrote:> Hi, > > Attached is the final version of the patch, adding the requested > FIXME. If this is ok, can > somebody check it in ?Just a possible issue from inspection: does this handle 64-bit operations correctly on x86? Unless I'm missing something, it seems likely to lead to a mysterious error for such operations. Also, does DAGCombiner know not to touch operations with a known user of the non-primary return value? This seems like it could lead to strange code in some cases, and it seems likely to be the cause of the crash Zoltan mentioned... would it be better to use target-specific nodes for the calculation? -Eli