I'm not familiar with this syntax:
typename... MemoryKinds... and ids...
What is this called?
And is it supported across our compilers?

It's called a variadic template, and yes it's supported across our compilers (and already used in a couple other places in the jit)

How about just using two consts: emptyMemoryKindSet' and fullMemoryKindSet`

Updated, thanks for the suggestion; having emptyMemoryKindSet instead of memoryKindSet() at those uses reads much cleaner I think.

global heap => GCHeap/ByrefExposed

Fixed (and likewise for the rest of these)

You might want to mention that we really only support two kinds of memory and that if a third kind is every added then you need to examine the use of the variable byrefStatesMatchGcHeapStates.

I think I avoided baking that assumption into the places that use byrefStatesMatchGcHeapStates. I used a few patterns:

1 - places where output[GcHeap] and output[ByrefExposed] reference the same data structure:

  for (MemoryKind memoryKind : allMemoryKinds()) {
    if (memoryKind == GcHeap && statesMatch) {
      // already updated
      assert(memoryKind > ByrefExposed);
      assert(input[memoryKind] == input[ByrefExposed]); // and/or assert(output[memoryKind] == f(input[memoryKind]))
      continue;
    }
    output[memoryKind] = f(input[memoryKind]);
  }

2a - places where output[GcHeap] and output[ByrefExposed] need to be copies of each other:

  for (MemoryKind memoryKind : allMemoryKinds()) {
    if (memoryKind == GcHeap && statesMatch) {
      // copy result from ByrefExposed
      assert(memoryKind > ByrefExposed);
      output[memoryKind] = output[ByrefExposed];
      continue;
    }
    output[memoryKind] = f(input[memoryKind]);
  }

2b - other places where output[GcHeap] and output[ByrefExposed] need to be copies of each other:

  for (MemoryKind memoryKind : allMemoryKinds()) {
    if (memoryKind == GcHeap && statesMatch) {
      // already updated
      assert(memoryKind > ByrefExposed);
      assert(input[memoryKind] == input[ByrefExposed]); // and/or assert(output[memoryKind] == f(input[memoryKind]))
      continue;
    }
    output[memoryKind] = f(input[memoryKind]);
    if (memoryKind == GcHeap && statesMatch) {
      // copy over to ByrefExposed
      output[ByrefExposed] = output[memoryKind];
    }
  }

(2a was more convenient for some of the particular updates, 2b for others).

In all cases, if a third kind were added the loop would just process it independently of GcHeap and ByrefExposed, which I think is a good default position.

Does that address your concern, or am I misunderstanding the feedback?

Yes, thanks for the explaination

I would be willing to reorder m_refAnyClass to come before the loop

This would read better as
memoryHavoc = emptyMemoryKindSet();

Also add emptyMemoryKindSet

GC andf Byref

GC and Byref

We track these during MinOpts?

No, perhaps the comment was misleading. I don't expect byrefStatesMatchGcHeapStates to be checked in minOpts, but leaving it unset didn't seem right. I've updated the comment to

        // In minopts, we don't explicitly build SSA or value-number; GcHeap and
        // ByrefExposed implicitly (conservatively) change state at each instr.

Is that better?

You could reduce what we print in the dumps by adding:

if (byrefStatesMatchGcHeapStates) { break; }

I actually had it like that at first, but regretted it as soon as I started debugging this stuff. Perhaps if that meant that ByrefExposed (or GcHeap) were just never mentioned in the dumps of byrefStatesMatchGcHeapStates methods, it would be useful, but since some of the liveness code that runs before statesMatch has been computed will end up dumping both, it just turns out confusing; I think better a more understandable dump than a smaller one, if we have to choose (and it seems in this case we have to choose).

You could reduce what we print in the dumps by adding:

if (byrefStatesMatchGcHeapStates) { break; }

emptyMemoryKindSet

if (byrefStatesMatchGcHeapStates) { break; }

if (byrefStatesMatchGcHeapStates) { break; }

Can memoryHavoc actually be different for GcHeap vs ByrefExposed?

If so it would be useful to have a test case in this area

Since we assume all object field stores, static field stores, and array element stores alias all byref loads, we treat them as ByrefExposed-havoc (but not GcHeap-havoc). So every loop that has these but not GcHeap-havoc will have one kind of havoc but not the other.

typo corresopnding

Should we try to keep the old assert?
assert(compCurBB->bbHeapDef);

This code used to assign fgCurHeapVN but doesn't anymore. Now it just returns the new value number, which then gets passed to recordGcHeapStore to do the update to fgCurMemoryVN[GcHeap], and that function does have the updated assert.

Now do the same for both kinds of memory.

Do we have to twice the work here, or should we check the bool byrefStatesMatchGcHeapStates

We do check byrefStatesMatchGcHeapStates, on line 4512 and line 4572. It made more sense to push them into the else branch of the conditional in the loop, because the if branch needs to do the same processing for both memoryKinds regardless.

Sometimes we query the memory ssa map in an assert

GC or Byref

Should this say of address exposed locals rather than untracked locals

should this say aren't address exposed rather than aren't tracked

untracked => address exposed

GC/Byref

GC/Byref heaps

Yes, thanks for the explaination