llvm dev - Dec 2009 - [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.

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.

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 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).

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