This is an archive of the discontinued LLVM Phabricator instance.

[IR] Require intrinsic struct return type to be anonymous
ClosedPublic

Authored by nikic on Mar 25 2022, 3:38 AM.

Details

Summary

This is an alternative to D122376. Rather than working around the problem, this patch requires that struct return type in intrinsics are anonymous/literal and adds auto-upgrade code to convert existing uses of intrinsics with named struct types.

This also fixes https://github.com/llvm/llvm-project/issues/37891.

Diff Detail

Event Timeline

nikic created this revision.Mar 25 2022, 3:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2022, 3:38 AM
nikic requested review of this revision.Mar 25 2022, 3:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2022, 3:38 AM
nikic added inline comments.Mar 25 2022, 3:42 AM
llvm/lib/IR/AutoUpgrade.cpp
591

This is a remangling upgrade, but previously was done manually to preserve the named struct type. Now we should use the generic code.

llvm/lib/IR/Function.cpp
1487

This is a bit of an ugly edge case, because "externref" type used by some wasm intrinsic is defined as pointer to empty struct, where "empty struct" is a named type in practice. I didn't bother handling this case, because this edge-case will go away with opaque pointers anyway, and as such doesn't seem worth addressing with additional machinery right now.

llvm/test/CodeGen/X86/2009-04-12-FastIselOverflowCrash.ll
8

The auto-upgrade does work fine for these tests, but they are testing FastISel and the resulting extract+insert+extract sequence is not supported by FastISel.

aeubanks accepted this revision.Mar 25 2022, 11:07 AM
aeubanks added a subscriber: aeubanks.

intrinsics-struct-upgrade.ll is failing on the buildbots

This revision is now accepted and ready to land.Mar 25 2022, 11:07 AM

I think dropping struct names from intrinsic return types is a bit unfortunate. I wonder if there's a way to keep them?

For context, I've been imagining the following transition for named structs:

  1. Opaque pointers. (This removes all cycles in the Type graph.)
  2. Refactor named structs into a pointer to "unnamed struct" (immutable) + "a name".
  3. "Remove" named structs, moving the name field to a map in Module (and, maybe, a map per-Function to override).
    • getType() always refers to an "unnamed struct".
    • Textual IR would print the name assigned by the owner (instruction checks map in Function (if we have that), then Module). Instruction::print() can do the same.
    • (Requires (1), which removes all cycles in the Type graph.)
  4. (All Types are now immutable. Refactor accordingly.)
  5. (Make get() and create() thread-safe for types by using a thread-safe insert-only data structure.)
  6. (various other things...)

(More context than needed/useful, probably; also, I'm aware that (3) has caveats / hasn't been proposed / etc.)

My point: once (2) is done, it would be trivial to check for two different named types whether they were structurally equivalent. At point, you could have module-specific names on structs returned from intrinsics.

I wonder if doing (2) now (before opaque pointers is done) would be possible/useful, and if it'd help keeping names here?

Or, maybe, names on struct types returned by intrinsics just aren't very valuable? (since they're rare / small / easy enough to reason about?)

nikic added a comment.Mar 26 2022, 2:33 AM

@dexonsmith In the end, the part we care about is that that there is a unique mapping between intrinsic names and intrinsic function types. Even if we represented named structs as a type containing an anonymous struct and a name, that would still mean we could have two different function types for one intrinsic name. The only way to avoid that would be to actually include the struct type in intrinsic mangling, so we'd have a different intrinsic for (say) llvm.sadd.with.overflow.i32.s_anon returning an anonymous struct and llvm.sadd.with.overflow.i32.s_0 returning a %0 named struct. That's possible, but I don't think its worthwhile.

Conversely, if we move towards only having anonymous structs and a name assigned in a separate map, then that name is no longer part of the Type, and we only have the one intrinsic signature. The name is just a question of pretty rendering. Though TBH I am not sure how much sense that approach makes -- I think named types are primarily of interest if you can use different names for the "same" type, while this would limit you to one.

Generally, I'd say that struct types in LLVM are on the way out. Opaque pointers make the first major step towards that, and if we could get rid of all the other unnecessary type uses in LLVM (GEP, alloca, byval, etc) the only remaining use for struct types would be functions with multiple return values. But well, we're still rather far away from this bright future :)

Or, maybe, names on struct types returned by intrinsics just aren't very valuable? (since they're rare / small / easy enough to reason about?)

Yeah, I think when it comes to IR intrinsics, struct returns are generally only used to return two values. I don't think people use named return types for this in practice anymore, because it doesn't really work for non-trivial situations (i.e. whenever LLVM creates new intrinsic calls, as it does with with.overflow intrinsics).

Thanks for thinking it over; SGTM. (I haven't reviewed details of this patch, but don't let my comment block landing it.)

I think named types are primarily of interest if you can use different names for the "same" type, while this would limit you to one.

IMO, named types also add value when describing nested structures. E.g., this:

declare {i32, {i32, {i32, i32}, {i32, i32}, i32}, i32, {i32, i32}} @thing2er()

is harder to read than this:

%i32pair = {i32, i32}
%thing1 = {i32, %i32pair, %i32pair, i32}
%thing2 = {i32, %thing1, i32, %i32pair}

declare %thing2 @thing2er()

IMO, after opaque pointers land, type names will just be helpful comments for debugging IR, like value names; not really a core part of the type system. But I take your point that it's useful to have multiple names for the same type. I'll keep thinking; maybe there's a good way to keep that.

nikic added a comment.Mar 28 2022, 1:32 AM

Hrm, unfortunately this fails clang/test/CodeGenCUDA/texture.cu, which does:

__attribute__((device)) v4f tex2d_ld(texture<float, 2, ElementType>, float, float) asm("llvm.nvvm.tex.unified.2d.v4f32.f32");
__attribute__((device)) v4f tex2d_ld(texture<float, 2, NormalizedFloat>, int, int) asm("llvm.nvvm.tex.unified.2d.v4f32.s32");

TIL about the asm attribute, though thankfully clang docs are very explicit about this usage being unsupported:

While it is possible to use this attribute to name a special symbol used internally by the compiler, such as an LLVM intrinsic, this is neither recommended nor supported and may cause the compiler to crash or miscompile. Users who wish to gain access to intrinsic behavior are strongly encouraged to request new builtin functions.

I'm not entirely sure how to resolve this -- possibly the right action here is to simply delete the test, as it's an unsupported usage. An alternative would be to do intrinsic auto-upgrade during frontend codegen (this is something rustc does for example), but given the fact that this should only be relevant for this asm construct, and that such usage is explicitly unsupported, we probably shouldn't go out of our way to make it work.

nikic updated this revision to Diff 418812.Mar 29 2022, 12:51 AM
nikic added subscribers: tra, hliao.

Mark clang/test/CodeGenCUDA/texture.cu as XFAIL.

cc @hliao @tra in case you want to export builtins for these intrinsics before I land this.

tra accepted this revision.Mar 29 2022, 11:49 AM

I agree that that texture.cu test is broken. Using inline IR asm is a rather questionable thing to do. I'll need to figure out a better way to test it, but it should not stop this patch. I think in practice we're not relying on those intrinsics and use inline asm to do texture fetches, so the change should not affect end users.

This revision was landed with ongoing or failed builds.Mar 30 2022, 12:51 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 12:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Hello,

No idea what is happening yet but I've seen some pretty nasty memory consumption by opt that I bisected to this patch.

Any idea what could be going on?

I'll see if I can see if I can extract some reproducer.

uabelho added a comment.EditedMar 30 2022, 7:18 AM

Hello,

No idea what is happening yet but I've seen some pretty nasty memory consumption by opt that I bisected to this patch.

Any idea what could be going on?

I'll see if I can see if I can extract some reproducer.

It seems like it's the actual reading of the input file that hangs and just eats more and more memory. So

opt foo.bc -o foo.opt.bc

hangs, and this happens when the input contains a call to an intrinsic we've added downstream that returns a named struct type.

If I extract ll output from clang it looks like

%struct.__divm32_t_tag = type { i32, i32 }
[...]
  %0 = call %struct.__divm32_t_tag @llvm.phx.divm.u32.s_struct.__divm32_t_tags(i32 2, i32 9), !dbg !10

and opt happily reads that, but if I output bc from clang and then feed opt it hangs.

Do we need to update our intrinsic definitions in some way?

nikic added a comment.Mar 30 2022, 7:55 AM

@uabelho You shouldn't need to update intrinsic definitions (though you might want to update how you codegen the intrinsics). It sounds like the auto-upgrade code is going into an infinite loop in your case, but it's not really obvious to me how/why that would happen (or why it would happen only for bitcode but not IR).

nikic added a comment.Mar 30 2022, 7:57 AM

Oh wait, do I see correctly that your intrinsic returns an overloaded type that happens to be a struct, rather than returning a struct defined in the intrinsic definition? I hadn't considered that case, and it's possible the auto-upgrade code is broken for that.

@uabelho Can you please check whether https://github.com/llvm/llvm-project/commit/d6887256c2cae1b1b721bd47459be6d86003db6f fixes the problem you're seeing?

Hi @nikic

Yes, that patch solves the problem. Great, thanks!

This comment was removed by goncharov.