Javier Martinez
2009-Dec-04 00:09 UTC
[LLVMdev] Possible bug in ExpandShiftWithUnknownAmountBit
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 do. Do you have a suggestion? Thanks, Javier On Thu, 03 Dec 2009 22:38:44 +0100, Duncan Sands <baldrick at free.fr> wrote:> 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.
Duncan Sands
2009-Dec-04 08:47 UTC
[LLVMdev] Possible bug in ExpandShiftWithUnknownAmountBit
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 without a testcase :) Also, I noticed a subtle bug. I added a comment about it in commit 90564. Does this impact your target? It's not clear how best to fix it (it is easy to fix inefficiently). Ciao, Duncan.
Duncan Sands
2009-Dec-04 20:42 UTC
[LLVMdev] Possible bug in ExpandShiftWithUnknownAmountBit
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).
Javier Martinez
2009-Dec-04 21:59 UTC
[LLVMdev] Possible bug in ExpandShiftWithUnknownAmountBit
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 some comments added to check-in 90564. In our architecture shifting by 0 doesn't cause any problems but the comment might be valid for others. Thanks, Javier On Fri, 04 Dec 2009 21:42:36 +0100, Duncan Sands <baldrick at free.fr> wrote:> 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).
Nick Lewycky
2009-Dec-05 02:21 UTC
[LLVMdev] Possible bug in ExpandShiftWithUnknownAmountBit
Duncan Sands wrote:> 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 without a testcase :)It would be fantastic if someone could take the lead and create our first codegen or target unittest. You could write a small C++ program under unittests/ which #include "../../lib/Target/whatever" as needed to call directly into the code with the bug. Nick
Reasonably Related Threads
- [LLVMdev] Possible bug in ExpandShiftWithUnknownAmountBit
- [LLVMdev] Possible bug in ExpandShiftWithUnknownAmountBit
- [LLVMdev] Possible bug in ExpandShiftWithUnknownAmountBit
- [LLVMdev] Possible bug in ExpandShiftWithUnknownAmountBit
- [LLVMdev] Possible bug in ExpandShiftWithUnknownAmountBit