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 
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
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.
1 Like
Yes, that’s still the plan. We’re just being a bit conservative here, because once we migrate test coverage to opaque pointers, it’s likely that typed pointer support will bitrot. This needs to happen at some point, but it’s preferable if it happens after we’ve addressed issues caused or exposed by the switch to opaque pointers.
Though from the responses, I think we’re about read to move forward with the migration. Let’s try to land the default to opaque pointers on the LLVM side next week, and start updating tests afterwards. I do already have some scripts written to help with migration, but haven’t extensively tested them yet.
Excuse me, asking a basic question, I’m a newcomer for this topic . Why do we need to change typed pointer into opaque pointers? I don’t know if there’s a link description.
See the second link in the first post here.
1 Like
Part of the regressions come from capture tracking returning more pessimistic results with opaque pointers, due to how the current limit works.
At the moment, we cap the number of uses to explore per-value, not for all uses during the traversal. So without opaque pointers, we might have bit casts or 0-geps in the chain, and for each we will explore up to 20 uses.
With opaque pointers, the underlying object has more uses, because the bit casts and other instructions are gone, so we more likely run into the 20 use limit for that pointer.
One solution would be to apply the limit to all visited uses and increase the limit as well: ⚙ D126236 [CaptureTracking] Increase limit but use it for all visited uses.
Okay, this took a bit longer than expected, but opaque pointers are now enabled by default on the LLVM side (as of ⚙ D126689 [IR] Enable opaque pointers by default). Let me know if I missed any buildbot failures or you encounter other issues.
If you have a frontend that isn’t ready for opaque pointers yet, you’ll want to add a call to Ctx.setOpaquePointers(false) to restore the previous behavior.
I’ve started working on the migration of tests to opaque pointers. If anyone wants to help, I’m using this script to perform the updates: https://gist.github.com/nikic/98357b71fd67756b0f064c9517b62a34 I’m updating the gist with more hacks improvements from time to time.
It would be great if newly added tests use opaque pointers, unless there is some reason to do otherwise (e.g. the test is intended for an upstream or downstream backport).
Hi,
Is there any time frame for the test case migration?
The latest change for AggressiveinstCombine has no code path for typed pointers, so having the typed pointer code path seems to become useless.
Kai
There hasn’t been a lot of progress on the test migration yet – some test directories in LLVM have been converted to use opaque pointers, but this is still only a relatively small subset of all tests. Any help is welcome here. (Test conversions that don’t have any noteworthy optimization differences between typed and opaque pointers can be committed directly, without review.)
Last week I have started working on migrating Clang tests to use opaque pointers. I hope that at least this part can be done relatively soon, at which point the -no-opaque-pointers flag in Clang will be dropped, and all the Clang-side support code for typed pointers can be removed. Removal of typed pointer support from LLVM itself is probably still a while away.
1 Like
Some feedback on the migration script (which is a great help, thank you!):
- I think comment lines which begin at the first column and do not contain a CHECK line should not be touched, because this is most likely a user comment. E.g. having some C code as comment:
; void test(float *) { ... } - In some files which I converted manually, a
getelementptrline having no typed pointer in it was wrongly rewritten when running the migration script on the file. - Pointer to globals or functions returning pointers are written as
ptr@name, e.g.declare ptr@strcpy(ptr %dest, ptr %src). It’s correct but personally I would insert a space after the firstptr. - In test case
CodeGen/SystemZ/Large/branch-01.lla label and an i32 value got the same name%6598assigned, causing a parsing error.
Thanks again for providing the script!
As a minor update, ⚙ D139940 [Bitcode] Remove auto-detection for typed pointers removed auto-detection for typed pointers for bitcode. To avoid an auto-upgrade to opaque pointers, it’s now necessary to pass -opaque-pointers=0.
We’re still about 2000 failing tests away from being able to do the same for textual IR.
The main problem are Clang tests though. Clang codegen tests are extremely heterogeneous, so it takes a lot of time to update them. It does not seem like Clang developers have a policy of using update_cc_test_checks.py for all tests, as we do on the LLVM side.