hard values in SequentialType::indexValid () method

This method is defined in:
Lib/VMCore/Type.cpp
And it makes hard assumption that size of integer is 32 or 64.
This gives us trouble because we have 16 bit integer.
Is there a reason for this assumption? Or we can just add the 16-bit
integer to it as well?

Thanks
Alireza Moshtaghi
Senior Software Engineer
Development Systems, Microchip Technology

Hi Alireza,

The reason for this is that the getelementptr instruction is spec'd to take either an i32 or i64. We could extend this, but it probably won't make a difference in practice. If the front-end generates code like:

%tmp = sext i16 %idx to i32
%tmp2 = getelementptr float* %P, i32 %tmp

Then the llvm code generator will squish the sign extension etc when it lowers GEP to pointer arithmetic.

-Chris

What is the reason for getelementptr being tied to only 32 or 64 bit
indexes? It seems to me that integer size would be better choice...

Anyways, this is getting in our way. What is your suggestion?

Thanks
Ali

From: llvmdev-bounces@cs.uiuc.edu [mailto:llvmdev-bounces@cs.uiuc.edu]

On

Behalf Of Chris Lattner
Sent: Monday, August 18, 2008 4:33 PM
To: LLVM Developers Mailing List
Subject: Re: [LLVMdev] hard values in SequentialType::indexValid ()

method

What is the reason for getelementptr being tied to only 32 or 64 bit
indexes?

As usual, the answer is "that is all anyone has needed so far" :slight_smile:

Anyways, this is getting in our way. What is your suggestion?

Two options: 1) change LLVM IR to allow it, or 2) just insert sign/zero extension instructions.

Does this actually affect the code being generated in practice?

-Chris

Two options: 1) change LLVM IR to allow it, or 2) just insert sign/
zero extension instructions.

Does this actually affect the code being generated in practice?

Inserting sign/zero extension instructions actually do tend to increase
the generated code pretty dramatically on our target. So I prefer option
(1)

Does this change have dramatic affect on LLVM IR?

PS.
Currently (in our local code repository) we have modified the
indexValid() method to also allow i16 and so far our test cases are
going through.
I also tried :

float *p1;
float *p3;
short i;
...
p1 = p2 + i;

to generate your example:

%tmp = sext i16 %idx to i32
%tmp2 = getelementptr float* %P, i32 %tmp

On x86; but the sign extension does not get eliminated while lowering
the GEP.
Maybe I understand better if you could give me a C code that shows the
difference.

Thanks
Ali

Two options: 1) change LLVM IR to allow it, or 2) just insert sign/
zero extension instructions.

Does this actually affect the code being generated in practice?

Inserting sign/zero extension instructions actually do tend to increase
the generated code pretty dramatically on our target. So I prefer option
(1)

I realize that sign extensions have non-zero impact when they exist in the .s file. My assertion is that these will be zapped by the dag combiner. Have you actually tried this?

On x86; but the sign extension does not get eliminated while lowering
the GEP.

That is because X86 has a 32-bit pointer. If you have a 16-bit pointer, a sign extend from 16 bits to 16 bits will get deleted.

-Chris

I realize that sign extensions have non-zero impact when they exist in
the .s file. My assertion is that these will be zapped by the dag
combiner. Have you actually tried this?

Yes, I have and they work just fine for us. I misunderstood your first
email, I thought you are pointing at a problem, while you are actually
saying that there is no problem.

So back to my original question...
Would adding i16 to indexValid() do the trick? Or we need to do more?

Thanks
A.

I realize that sign extensions have non-zero impact when they exist in
the .s file. My assertion is that these will be zapped by the dag
combiner. Have you actually tried this?

Yes, I have and they work just fine for us. I misunderstood your first
email, I thought you are pointing at a problem, while you are actually
saying that there is no problem.

Ok

So back to my original question...
Would adding i16 to indexValid() do the trick? Or we need to do more?

It's more tricky than that. The bc encoding uses one bit to distinguish between the i32 and i64 cases. There may also be implicit assumptions elsewhere that indexes are 32 or 64-bits. The best thing to do is to start making the change and see what breaks :). There are probably not very many places that depend on this.

-Chris