This is an archive of the discontinued LLVM Phabricator instance.

[IRBuilder] Use preferred target type for len argument of memory intrinsic functions
Needs RevisionPublic

Authored by kschwarz on Mar 17 2020, 7:02 AM.

Details

Summary

The IR builder hard-coded the type of the len argument of the memory function instrinsics to i64 for several builder functions.

During instruction selection of these intrinsics to the corresponding C library functions SelectionDAG ignores the type of the len argument and uses the targets preferred type.
GlobalISel however translates these calls using the actual types of the intrinsic, which will miscompile on 32-bit architectures.

Using the preferred type when creating the intrinsic call in the first place fixes this issue.

Diff Detail

Event Timeline

kschwarz created this revision.Mar 17 2020, 7:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2020, 7:02 AM

So what happens if i manually create such IR, or create such IR without using IRBuilder?
It will again compile fine via SelectionDAG and will be miscompiled via GlobalISel, correct?
Don't you then want to harden -verify to barf on such mismatch, too?

Correct, it will still miscompile even with this change.
I see two options: either 1) extend GlobalISel to select to the appropriate type, or 2) declare this as illegal LLVM IR in the first place.

  1. will introduce an implicit truncation of the i64 argument (same as SelectionDAG does today). In practice this seems to work, but it feels broken
  2. I'm not sure whether it should be illegal to create an i64 version of the llvm.mem* intrinsics on 32-bit architectures.

Thoughts?

llvm-dev might be a better place for that question, wider coverage.

I guess the logical rule would be that the bit-width of the size has to match the minimum of the bit-widths of the address spaces used with the intrinsic. (Note that llvm.memcpy etc. can take pointers into two different address spaces, potentially of different widths.) That might be pretty disruptive for existing code, though.

kschwarz updated this revision to Diff 269789.Jun 10 2020, 4:09 AM

Brining this up again

Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2020, 4:09 AM

I brought this to the mailing list (http://lists.llvm.org/pipermail/llvm-dev/2020-March/140032.html) but didn't get an answer there.

@arsenm, have you any comments regarding @rjmccall's last comment regarding the sizes of pointers in different address spaces?

I would harden the verifier and declare the i64 versions of mem intrinsics illegal on 32 bits architectures.
Also the intrinsic sections of LangRef should be updated accordingly.

I brought this to the mailing list (http://lists.llvm.org/pipermail/llvm-dev/2020-March/140032.html) but didn't get an answer there.

@arsenm, have you any comments regarding @rjmccall's last comment regarding the sizes of pointers in different address spaces?

I've always use the minimum of the two sizes, so I agree

arsenm requested changes to this revision.Aug 17 2023, 3:56 PM

I think any size type should be valid for the intrinsic. Legalization should have to cast the type to the target libcall if that's how it chooses to implement the lowering

This revision now requires changes to proceed.Aug 17 2023, 3:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2023, 3:56 PM
arsenm added inline comments.Aug 17 2023, 3:56 PM
llvm/include/llvm/IR/IRBuilder.h
438–446

This change on its own might be ok but in the context of what you are trying to solve it is not