This is an archive of the discontinued LLVM Phabricator instance.

[InferAlignment] Enable InferAlignment pass by default
ClosedPublic

Authored by 0xdc03 on Aug 23 2023, 5:25 AM.

Diff Detail

Event Timeline

0xdc03 created this revision.Aug 23 2023, 5:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 5:25 AM
0xdc03 requested review of this revision.Aug 23 2023, 5:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 5:25 AM
0xdc03 updated this revision to Diff 552751.Aug 23 2023, 9:13 AM
  • Move pipeline tests to the correct place
0xdc03 edited the summary of this revision. (Show Details)Aug 23 2023, 9:17 AM
0xdc03 updated this revision to Diff 554224.Aug 29 2023, 2:36 AM
  • Rebase on main
aeubanks added inline comments.
llvm/test/Transforms/PhaseOrdering/X86/excessive-unrolling.ll
135 ↗(On Diff #556286)

any idea why this regressed?

0xdc03 added inline comments.Sep 18 2023, 9:48 AM
llvm/test/Transforms/PhaseOrdering/X86/excessive-unrolling.ll
135 ↗(On Diff #556286)

Loop unrolling causes generation of instructions after InferAlignment runs, so this is not caught by it.

0xdc03 updated this revision to Diff 556959.Sep 18 2023, 9:49 AM
  • Rebase on main
  • Update tests following change in D158529
0xdc03 edited the summary of this revision. (Show Details)Sep 18 2023, 9:50 AM
aeubanks added inline comments.Sep 18 2023, 9:51 AM
llvm/lib/Passes/PassBuilderPipelines.cpp
1285

can this also be replaced with the new pass?

0xdc03 added inline comments.Sep 19 2023, 3:43 AM
llvm/lib/Passes/PassBuilderPipelines.cpp
1285

I do not believe so, because this uses SCEV information whereas the InferAlignment pass does not. It can probably be implemented in a later patch though.

0xdc03 updated this revision to Diff 557026.Sep 19 2023, 3:53 AM
0xdc03 edited the summary of this revision. (Show Details)
  • Fix broken test cases
nikic accepted this revision.Sep 19 2023, 12:03 PM

LGTM -- based on stats this should be low impact, so let's try it. This very runs the pass two times (before and after unrolling), which is probably more than we need longer term but for now should hopefully avoid regressions.

This revision is now accepted and ready to land.Sep 19 2023, 12:03 PM
This revision was automatically updated to reflect the committed changes.
nlopes added a subscriber: nlopes.Sep 20 2023, 1:50 PM

I'm confused...

This pass regresses alignment information in a lot of tests. I've also observed in when compiling real programs.
What's the reason for enabling this?

nikic added a comment.Sep 20 2023, 1:58 PM

I'm confused...

This pass regresses alignment information in a lot of tests. I've also observed in when compiling real programs.

The test regressions in this diff are just an artifact of inference moving into a different pass (the tests run instcombine as part of the pipeline, while is no longer responsible for alignment inference). If you see end-to-end regressions in alignment information, that would be a problem though. Do you have an example?

I'm confused...

This pass regresses alignment information in a lot of tests. I've also observed in when compiling real programs.

The test regressions in this diff are just an artifact of inference moving into a different pass (the tests run instcombine as part of the pipeline, while is no longer responsible for alignment inference). If you see end-to-end regressions in alignment information, that would be a problem though. Do you have an example?

Ah, I see.
No, didn't see end-to-end regressions. I was looking somewhere in the middle of the pipeline only.
But this may impact alias analysis, depending on how often and there this new pass runs. Instcombine runs more often than this pass. We need to monitor for regressions.

alexfh added a subscriber: alexfh.Sep 26 2023, 9:03 AM

We've started seeing ubsan unaligned access errors after this commit. I'm not yet sure the code is correct, so just a heads up for now.