similar to: [LLVMdev] [Patch] Fix for ExpandShiftWithUnknownAmountBit when shift amount is zero

Hi, > I'm working in adding support for 64-bit integers to my target. I'm using > LLVM to decompose the 64-bit integer operations by using 32-bit registers > wherever possible and emulating support where not. When looking at the bit > shift decomposition I saw what seems to be a bug in the implementation. The > affected function is ExpandShiftWithUnknownAmountBit in >

On Mon, Nov 30, 2009 at 7:22 PM, Javier Martinez <javier at jmartinez.org> wrote: > Hello, > > I'm working in adding support for 64-bit integers to my target. I'm using > LLVM to decompose the 64-bit integer operations by using 32-bit registers > wherever possible and emulating support where not. When looking at the bit > shift decomposition I saw what seems to be a

Hi Javier, > Just wondering if you had a chance to look at the expansion. I wrote my own fix and committed it in revision 90482. Do you have a testcase I can add to the testsuite (preferably not an execution test, but rather something that checks the assembler being generated)? Ciao, Duncan.

Hello, I'm working in adding support for 64-bit integers to my target. I'm using LLVM to decompose the 64-bit integer operations by using 32-bit registers wherever possible and emulating support where not. When looking at the bit shift decomposition I saw what seems to be a bug in the implementation. The affected function is ExpandShiftWithUnknownAmountBit in LegalizeIntegerTypes.cpp.

Hi Duncan, The problem is the implementation of the expansion. Perhaps an example can help illustrate better. Take the case of a 64-bit integer shifted left by say 6 bits and is decomposed using 32-bit registers. Because 6 is less than the 32 (the register size) the resulting low part should be equal to the source low part shifted left by 6 bits. The current implementation places a zero

Hi Duncan, Just wondering if you had a chance to look at the expansion. Thanks, Javier On Tue, 01 Dec 2009 20:20:14 +0100, Duncan Sands <baldrick at free.fr> wrote: > Hi Javier, > >> It seems that the code you pasted came from the function >> ExpandShiftByConstant and indeed it looks correct. In my example I used 6 >> as the shift amount but forgot to mention that

Duncan, Thanks for committing a fix. I see that you fixed a bug in my patch with the HiL in the case of an SRA. The issue with a test case is that it will depend on the target to expose it. Like you noted in the check-in comment x86 doesn't expose the bug. The target I'm working on isn't public. Short of writing a new one for the purpose of a test case I don't know what else to

Hello LLVM, A recent commit creates the isIntDivCheap() target query. http://reviews.llvm.org/D12082 The current approach has a couple shortcomings. First, when targets decide divide is cheap, the DAGCombiner ignores obvious power-of-2 optimizations. In the targets I know, shifts are cheaper than divides in both speed and size. The target cannot see the value in the isIntDivCheap() call, so

Hi Javier, > It seems that the code you pasted came from the function > ExpandShiftByConstant and indeed it looks correct. In my example I used 6 > as the shift amount but forgot to mention that it's stored in a register. I see, sorry I didn't read your email more carefully. It does look like ExpandShiftWithUnknownAmountBit is bogus - at a glance it looks like the Cmp condition

Duncan, It seems that the code you pasted came from the function ExpandShiftByConstant and indeed it looks correct. In my example I used 6 as the shift amount but forgot to mention that it's stored in a register. Otherwise ExpandShiftWithUnknownAmountBit wouldn't get called. Below is the execution of DAGTypeLegalizer::ExpandIntRes_Shift() using my example showing how

PS: For a small optimization, in the case where Amt is bigger than 32 (or whatever NVTBits is) you might want to use an "and" to mask off the top bits of Amt rather than subtracting 32 (if Amt is 64 or greater then the result of the shift was undefined anyway, so it is ok to mask off all the upper bits).

Hi Javier, > The issue with a test case is that it will depend on the target to expose > it. Like you noted in the check-in comment x86 doesn't expose the bug. The > target I'm working on isn't public. Short of writing a new one for the > purpose of a test case I don't know what else to do. Do you have a > suggestion? in that case I guess we will have to live

Hi Javier, > I don't know if the optimization would help us much. Our architecture > performs integer shifts and subtractions at the same speed. I think the > optimization is limited to the case where NVBits is a power of two. it is irrelevant whether it is a power of two or not. Anyway, since you are the only user, and this is not helpful for you, I guess we can forget it :) >

Hi Duncan, Maybe I'm beating a dead horse but I wanted to see if understood correctly your optimization to avoid doing a subtraction. The way I understand it the idea is to mask out the high bits of Amt which in some cases is equivalent to subtracting a value from Amt. The cases where this holds is when the value subtracted from Amt is a power of two. This value is half the original register

Hi Javier, > The problem is the implementation of the expansion. Perhaps an example > can help illustrate better. Take the case of a 64-bit integer shifted > left by say 6 bits and is decomposed using 32-bit registers. Because 6 > is less than the 32 (the register size) the resulting low part should be > equal to the source low part shifted left by 6 bits. The current >

Hi Duncan, I don't know if the optimization would help us much. Our architecture performs integer shifts and subtractions at the same speed. I think the optimization is limited to the case where NVBits is a power of two. That is the case for all current value types but there's a separate thread where someone is proposing adding i20 as a native type. In your previous email you mentioned

The LegalizeDAG.cpp file has this code in SelectionDAGLegalize::PromoteNode: case ISD::BSWAP: { unsigned DiffBits = NVT.getSizeInBits() - OVT.getSizeInBits(); Tmp1 = DAG.getNode(ISD::ZERO_EXTEND, dl, NVT, Tmp1); Tmp1 = DAG.getNode(ISD::BSWAP, dl, NVT, Tmp1); Tmp1 = DAG.getNode(ISD::SRL, dl, NVT, Tmp1, DAG.getConstant(DiffBits, TLI.getShiftAmountTy()));

On Dec 22, 2009, at 2:38 PMPST, Bill Wendling wrote: > The LegalizeDAG.cpp file has this code in > SelectionDAGLegalize::PromoteNode: > > case ISD::BSWAP: { > unsigned DiffBits = NVT.getSizeInBits() - OVT.getSizeInBits(); > Tmp1 = DAG.getNode(ISD::ZERO_EXTEND, dl, NVT, Tmp1); > Tmp1 = DAG.getNode(ISD::BSWAP, dl, NVT, Tmp1); > Tmp1 = DAG.getNode(ISD::SRL, dl,

I'm getting the following error when trying to SDIV integers greater than 128bit (on an AMD64 target). LegalizeIntegerTypes.cpp:2047: void llvm::DAGTypeLegalizer::ExpandIntRes_SDIV(llvm::SDNode*, llvm::SDValue&, llvm::SDValue&): Assertion `LC != RTLIB::UNKNOWN_LIBCALL && "Unsupported SDIV!"' failed. Stack dump: 0. Running pass 'X86 DAG->DAG

LLVM (via clang) currently translates target intrinsics to generic IR whenever it can. For example, on x86 it translates _mm_loadu_pd to a simple load instruction with an alignment of 1. The backend is then responsible for translating the load back to the corresponding machine instruction. The advantage of this is that it opens up such code to LLVM's optimizers, which can theoretically speed