Indeed.

Yes, it did: the overload which has the return type as first argument.

In D157492#4577657, @tra wrote:

I think we're commingling two different things here.

• representing a value as a string
• printing something out for debugging purposes.

While for debugging we may usually want to do both, it would be very useful to be able to use them independently. That would allow doing things like dump("got a pair of values [" # !repr(value_A) # " : " # !repr(value_B # "]")).

We could add some glue to dump/!dump() so that they wrap non-string arguments into !repr() automatically.

It would be nice to have a simple test case that shows the final assembly.

It's a tail call, isn't it? Does it have the same restriction as an LLVM IR tail call, that allocas in the caller cannot be accessed by the callee?

It seems this comes down to this question: Should the type properties be per type name or should they be per type?

As shown by the many related revisions, this is clearly a good idea.

In D150370#4440349, @arsenm wrote:

Missing all the assembler tests (but I guess they come with the bitcode parts?)

I think this larger context deserves a wider discussion

Rebase

Rebase

Main change is removal of sdata::Symbol in favor of plain StringRef

Semi-related, I think we should start interpreting i1 argument values as SGPR masks (i1 inreg would be extended to sreg32)

Move SchemaField into this patch

Move SchemaField out of this patch

In D150370#4435919, @nikic wrote:

In D150370#4435746, @nhaehnle wrote:

More generally, I'm not happy that this new concept is being introduced as part of the target type implementation. This doesn't really seem helpful for this specific use case (it makes the implementation substantially more complex rather than simpler). It may well make sense as part of some larger context, but I think this larger context deserves a wider discussion (probably on discourse) to clarify what the goals of this abstraction are and make sure we have a good design for it.

Sure. It's a bit of a chicken-and-egg thing. I do think this approach is better than the piecemeal addition of things to bitcode reader/writer, which is rather subtle code that I don't think too many people understand, but at the same time, working on big changes is frustrating unless you have clear line-of-sight to actually getting something useful upstream. So I started with this small thing quite consciously on purpose.

I have a WIP change locally that leverages the same infrastructure for "extended metadata", and at least an early minimal version that does something interesting is something that I think I can put up reasonably soon so that the discussion doesn't end up overly abstract. I would also like to do the same thing for "extended instructions", but that's still a bit further out.

FWIW, I do think this is pretty reasonable when seen as a pure IR/bitcode abstraction. I wouldn't have concerns if this were (initially at least) represented as std::pair<StringRef, sdata::Value> and only used as part of reading/writing. That use case doesn't really need any of the Symbol machinery -- we get these as strings in IR/bitcode anyway, and I don't think there is a substantial performance difference between interning the strings and then comparing IDs compared to directly comparing to a small handful of strings, during parsing only.

The design you have makes a lot more sense if the sdata is supposed to be used as part of the in-memory IR as well, in which case the Symbols are important for more efficient access -- but I'm not sure if they are ideal in that context either. That would still require that we have Symbols and Values (std::variant, ugh) in the representation. Ideally we'd just have a C++ struct with properly typed members, and sdata only being used for the purposes of serializing/deserializing those structs using a general mechanism, without requiring new asm/bitcode support each time.

Drop symbols-as-values for now

• Drop symbols-as-values for now
• LangRef clarifications

Drop symbols-as-values for now

I am very concerned about making this a global structure, rather than something bound to the context.

More generally, I'm not happy that this new concept is being introduced as part of the target type implementation. This doesn't really seem helpful for this specific use case (it makes the implementation substantially more complex rather than simpler). It may well make sense as part of some larger context, but I think this larger context deserves a wider discussion (probably on discourse) to clarify what the goals of this abstraction are and make sure we have a good design for it.

Could you please add a test where a target type is first used and later type info for it is provided? I'd like to make sure this is an error.

Missed a spot...

Rebase, mostly changes related to APInt bitwidth being preserved.

Rebase, layout type defaults to void

Add iN prefix to integers (and bool) in structured data values

I tend to agree with Matt here. *Current* graphics APIs don't allow users to write code that uses dynamic allocas, but since the stack size can be set externally there's no fundamental reason why supporting them should be impossible.

Thanks, I like this much more :)

The proliferation of the ConvergenceControlUsage makes me a bit uncomfortable. It adds the overhead of an additional iteration over entire functions every time a function is inlined or perhaps the inlining is even attempted. And the only thing we get in exchange is a safety check against mixing controlled and uncontrolled convergent operations. Is it really worth it? I'd expect that in general, a compiler either always emits controlled or always emits uncontrolled operations, which means this isn't actually a scenario we need to worry about too much.

I think this is in pretty good shape. You may want to give it a bit more time in case more discussion shows up, but it's good for me.

I haven't looked at this in a while, sorry. I do plan to cycle back to it eventually, but right now my work scheduling is more stack-based than FIFO and this is fairly low down...

I'm not familiar with the code but the thrust of it makes sense. We should probably still try to CSE calls that are in the same basic block, though. Is that easy to do?

Thank you all for the reviews. I've addressed the remaining small comments as part of the commit.

My 2 cents:

Looks good to me apart from the comments.

Thanks, this looks pretty reasonable to me.

There could probably be a general section here to state that in the graphics calling conventions, inreg is used to denote arguments mapped to SGPRs, while other arguments are mapped to VGPRs.

Address some review comments

Thank you for taking a look. I'm going to address the low-level issues you pointed out immediately.

ping again after two weeks

I think this is a reasonable compromise until we can land D147116, but maybe give a few more days for others to chime in.

In D151341#4368708, @jhuber6 wrote:

We should be able to remove every convergent attribute under this special case, right.

This could be a top-level check in isSourceOfDivergence: if a source of divergence executes with a single lane, it de facto stops being a source of divergence.

Remove getStructType

Thanks for taking a look!

Ah, my bad. I was working with local changes for so long that I hadn't noticed this got lost somehow. Sorry about that. I will follow-up with an adjustment to my later typeinfo diff.

There's nothing right now that would prevent this. Though we could default the layout type to something other than nullptr, if that is preferred. Maybe void?

Okay, makes sense.

ping

This mostly LGTM, but it looks like some GlobalISel legalization is missing relative to SelectionDAG?

• explicitly list properties in LangRef
• add negative parser/assembler tests
• use tuple binding syntax

Remove two TODOs that have been done.

Major update, adding IR printing, parsing and bitcode writing/reading of the type info.

Absolutely do not globally reserve s[6:7] for such things. Low SGPRs are very often given special meaning in function signatures.

• rebase on top of preparatory commits
• remove else after return
• add an explicit test for binary ops