Enabling opaque pointers by default

Turns out our problem was fixed by ⚙ D124605 [SelectionDAGBuilder] Don't create MGATHER/MSCATTER with Scale != ElemSize. We have re-enabled opaque pointers for our staging/testing branches.

A new -fwhole-program-vtables on Windows issue popped up with opaque pointers that I’m investigating.

To not block the flip of --opaque-pointers I think we shouldn’t wait for all ToT clang users to switch to opaque pointers. We can probably flip the flag soon/now if nobody has objections.

Shouldn’t all the LIT tests be updated before --opaque-pointers can be turned on by default in LLVM? I’m still unclear about how we plan to migrate all the IR-based tests, for example are we going to update CHECK lines in existing tests first, and then eventually rewrite all the IR to avoid use of typed pointers? How are we staging that effort, exactly?

1 Like

Arthur recently implemented a change (⚙ D125735 [OpaquePtr][LLParser] Explicitly turn off opaque pointers if we see a star) to automatically disable opaque pointer mode if the IR contains typed pointers. With that change, we should be able to perform the switch without doing a large amount of test updates in advance.

Basically, the only things that would change is a) IR that doesn’t explicitly mention any pointer types would use opaque pointers and b) any users of LLVM as a library would get opaque pointers by default, unless they explicitly disable them in LLVMContext.

Ok, that answers my first question, thanks. However, I’d assume we still need a plan for updating the tests before support for typed pointers can be dropped. Have the details of that plan been worked out yet?

For reference, flipping the switch locally and running check-llvm gives 157 failures which is manageable.

After flipping the flag we can migrate tests in batches as we see fit, e.g. a directory at a time. As for actually doing the textual changes, previous related large scale changes related to opaque pointers (e.g. adding a type parameter to byval) I believe just used some sed or some python script. We should be able to come up with something not too complicated that turns T * into ptr for the vast majority of cases.

Then once we start removing support for typed pointers and updating passes to not worry about no-op bitcasts, we’ll have to remove the no-op ptr to ptr bitcasts in tests, which may be trickier.

Well, flipping the switch doesn’t really remove support for typed pointers, so as long as the CHECK lines don’t specifically mention typed pointers, the tests could pass even if the IR contains typed pointers in them. My concern is mostly with the rewrite that’d be required to remove typed pointers all together. I know that’s further down the road, but not too far away it seems :slight_smile:

I know someone was asking in IRC earlier today after a script to convert tests to use opaque pointers.

The other thing to note with respect to tests is that it’s possible that some tests end up duplicated, once to test typed pointers and the other to test opaque pointers. I’m not sure if there’s any of this in the LLVM monorepo, but I’ve definitely done similar things in other repositories. So any mass test changes should at least spend some effort making sure we aren’t creating useless duplicate tests in the process.

1 Like

I did :slight_smile:

Yes, if we wait longer this might happen. I thought typed pointers are dead and we should simply move all our tests to opaque ones, no?

We are still seeing a few notable performance regressions on some benchmarks with opaque pointers. But I should be able to track that down in the next few days.