Regularize LLVM code to remove use of non standard integer types in fptoui and fptosi intrinsics

[asudarsa]

It is possible that incoming LLVM IR conversion instructions convert floating point to non-standard integer types.
For e.g. %0 = call i2 @llvm.fptosi.sat.i2.f32(float %input)
Such types are not supported in SPIR-V. This PR cleans up such code and removes occurrence of non-standard integer types.

Thanks

[asudarsa]

Please review once marked ready. Thanks

[asudarsa]

This PR tries to address a very limited set of issues found due to use of non-standard integer types. A more rounded solution may be required in the long run. Tracked here: #481

Thanks

[maksimsab]

LGTM.

[maarquitos14]

LGTM.

[MrSidims]

Data layout doesn't match the input LLVM IR ;-)

[asudarsa]

will fix in upcoming patch. Thanks

[asudarsa]

I looked at the original testcase. Data layout is exactly the same as it is here. Can you please let me know how it does not match the input LLVM IR? Thanks

[MrSidims]

n8:16:32:64

There is no "2" and "4" which would stand for i2 and i4. Which means, that the test case is either written artificially or generated using some hacks. If the later wouldn't it makes sense to investigate frontend hacks instead of changing translation?

[MrSidims]

Or if not hacks, may be just aggressive inst combine should respect data layout?

[asudarsa]

As I mentioned earlier, the test case is a 'modified' version of an actual LLVM IR file created using upstream LLVM compiler (LLVM.org). I have not changed the datalayout or the instructions related to 'i2' uses.
I am trying to figure out if this might be the case of the optimization not handling data layout correctly.
I am however surprised that 'llvm-as' does not complain about this LLVM IR. And I am also sure the verifier code runs at some time during front-end compilation. However, it is quite possible that they do not try to 'verify' datalayout related issues.

I went a bit deeper. i see that there is a helper function 'isLegalInteger' in the datalayout. However, this particular optimization does not call this function to check for legality. I will try to get this answered in the community.

In the meanwhile, would it make sense to 'merge' this as is and then revert it if/when frontend fix is in place?

Thanks

[asudarsa]

It seems the entries in datalayout are expected to be more of a guide that strict rules. for example, It is not documented anywhere that i2 is illegal if it is not listed in the target datalayout, thought it does sound counter-intuitive. I think we might have to assume that this LLVM IR is legal and proceed with the change here.

Thanks

[etiotto]

It seems the entries in datalayout are expected to be more of a guide that strict rules. for example, It is not documented anywhere that i2 is illegal if it is not listed in the target datalayout, thought it does sound counter-intuitive. I think we might have to assume that this LLVM IR is legal and proceed with the change here.

Thanks

I agree. The fact the datalayout field does not explicitly "mention" i2 doesn't prevent the LLVM optimizer to use i2 as an intermediat type during optimization IMO. The LLVM language spec simply states that the " data layout string that specifies how data is to be laid out in memory". There isn't anything that says using i2 is illegal.

[asudarsa]

Hi @svenvh and @MrSidims

Are there any more open issues/concerns? I have verified with LLVM documentation that generation of data types not mentioned in the datalayout is not explicitly called out as illegal. Please let me know if there is any evidence contrary to this.
If there are no concerns, can you please provide approvals when convenient?

Thanks a lot for your insightful feedbacks on this PR.
Sincerely

[svenvh]

No more concerns from my side.

[asudarsa]

Thanks everyone for the reviews.