Page MenuHomePhabricator

Support intrinsic overloading on unnamed types
Needs ReviewPublic

Authored by jeroen.dobbelaere on Wed, Nov 11, 4:20 AM.

Details

Summary

This patch adds support for intrinsic overloading on unnamed types.

This fixes PR38117 and will also be needed for the Full Restrict Patches (D68484).

The main problem is that the intrinsic overloading name mangling is using 's_s' for unnamed types.
This can result in identical intrinsic mangled names for different function prototypes.

This patch changes this by adding a '.XXXXX' to the intrinsic mangled name when at least one of the types is based on an unnamed type, ensuring that we get a unique name.

Implementation details:

  • The mapping is created on demand and kept in Module.
  • It also checks for existing clashes and recycles potentially existing prototypes and declarations.
  • Because of extra data in Module, Intrinsic::getName needs an extra Module* argument and, for speed, an optional FunctionType* argument.
  • I still kept the original two-argument 'Intrinsic::getName' around which keeps the original behavior (providing the base name).
    • Main reason is that I did not want to change the LLVMIntrinsicGetName version, as I don't know how acceptable such a change is
    • The current situation already has a limitation. So that should not get worse with this patch.
  • Intrinsic::getDeclaration and the verifier are now using the new version.

Other notes:

  • As far as I see, this should not suffer from stability issues. The count is only added for prototypes depending on at least one anonymous struct
  • The initial count starts from 0 for each intrinsic mangled name.
  • In case of name clashes, existing prototypes are remembered and reused when that makes sense.

Diff Detail

Event Timeline

jeroen.dobbelaere requested review of this revision.Wed, Nov 11, 4:20 AM
fhahn added a comment.Wed, Nov 11, 4:24 AM

Is this the same idea as in D48541 (linked from PR38117), where we use LLVMContext to keep track of IDs for anonymous types? If that's the case, the same objections as for D48541 probably apply here as well. I think one of the problems is that the names we assign depend on the order different modules are loaded.

Is this the same idea as in D48541 (linked from PR38117), where we use LLVMContext to keep track of IDs for anonymous types? If that's the case, the same objections as for D48541 probably apply here as well. I think one of the problems is that the names we assign depend on the order different modules are loaded.

Somehow I missed that one before.. But yes, the current implementation will suffer the same problem. Moving the DenseMap from LLVMContext to Module should resolve that problem ?

jeroen.dobbelaere edited the summary of this revision. (Show Details)

This new version uses a different approach: we now work by 'uniquifying' the initial BaseName of an intrinsic mangled named when it is based on an anonymous struct.

As an example, the testcase from D68484#2372356 now results in:

%0 = type { i32 }
%1 = type { i32 }

define void @bar01(%0* %ptr0, %1* %ptr1) {
  %1 = call i8* @llvm.noalias.decl.p0i8.p0p0s_s.i64.0(%0** null, i64 0, metadata !0)
  %2 = call %0* @llvm.noalias.p0s_s.p0i8.p0p0s_s.i64.0(%0* %ptr0, i8* %1, %0** null, i64 0, metadata !0)
  store %0 zeroinitializer, %0* %2, align 4, !noalias !0
  %3 = call i8* @llvm.noalias.decl.p0i8.p0p0s_s.i64.1(%1** null, i64 0, metadata !3)
  %4 = call %1* @llvm.noalias.p0s_s.p0i8.p0p0s_s.i64.1(%1* %ptr1, i8* %3, %1** null, i64 0, metadata !3)
  store %1 zeroinitializer, %1* %4, align 4, !noalias !3
  ret void
}

; Function Attrs: argmemonly nounwind
declare i8* @llvm.noalias.decl.p0i8.p0p0s_s.i64.0(%0**, i64, metadata) #0

; Function Attrs: argmemonly nounwind speculatable
declare %0* @llvm.noalias.p0s_s.p0i8.p0p0s_s.i64.0(%0*, i8*, %0**, i64, metadata) #1

; Function Attrs: argmemonly nounwind
declare i8* @llvm.noalias.decl.p0i8.p0p0s_s.i64.1(%1**, i64, metadata) #0

; Function Attrs: argmemonly nounwind speculatable
declare %1* @llvm.noalias.p0s_s.p0i8.p0p0s_s.i64.1(%1*, i8*, %1**, i64, metadata) #1
jeroen.dobbelaere retitled this revision from Support intrinsic overloading on anonymous struct based types to Support intrinsic overloading on unnamed types.
jeroen.dobbelaere edited the summary of this revision. (Show Details)
jeroen.dobbelaere added reviewers: fhahn, chandlerc.

I would like to propose this version for review. As far as I am aware, this version should not suffer from the objections made in D48541. Also a testcase has been included.

jeroen.dobbelaere edited the summary of this revision. (Show Details)Fri, Nov 13, 6:09 AM

Can't really comment on whether this makes sense approach wise. For testing, I'd suggest to test that this is correctly handled in bitcode serialization/loading as well, and to drop the current hack in PredicateInfo and see if it works fine for that use case.

... For testing, I'd suggest to test that this is correctly handled in bitcode serialization/loading as well...

The test should be doing that already by assembling and passing the bitcode to llvm-dis ?

fhahn added a comment.Wed, Nov 18, 2:30 AM

I think we would also need tests for combining multiple modules with intrinsics with unnamed type.

If the module owns the numbering, can encounter name/type clashes, e.g. if 2 modules have unnamed types that are different. i.e. both have declare %0* @llvm.ssa.copy.p0s_s.0(%0* returned) ,but %0 is defined as different types in both modules ?

I think we would also need tests for combining multiple modules with intrinsics with unnamed type.

If the module owns the numbering, can encounter name/type clashes, e.g. if 2 modules have unnamed types that are different. i.e. both have declare %0* @llvm.ssa.copy.p0s_s.0(%0* returned) ,but %0 is defined as different types in both modules ?

This is indeed a good suggestion. I am adding a link test (and it does bring up a case that I did not foresee, so I am also fixing it).

I improved the patch to actually support linking.

Moved the call to 'forceRenaming' closer to the original location.