[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

I do not think there is a sensible way to keep [`upgrade-datalayout2.ll`](https://github.com/llvm/llvm-project/blob/main/llvm/test/Bitcode/upgrade-datalayout2.ll) working, with the way the upgrade logic is structured, and we should rethink that test. The change here intends to insert `-i128:128-` into x86 data layouts that do not have it. The goal of `upgrade-datalayout2.ll` is to test that data layouts that are not valid x86 data layouts do not get upgraded. However, I see no sensible logic by which we can say that in this particular case, we should not add it.

What's more, none of the data layout upgrades *ever* checked that the data layout was a valid x86 data layout, not even D67631 <https://reviews.llvm.org/D67631> which added this test: it is easy to construct data layout strings that are not valid x86 data layout strings, that would already be upgraded by that very first version of `UpgradeDataLayoutString`, despite what the test claimed to check. So if we regard it as a bug to upgrade invalid target data layout strings, this is a pre-existing bug. Alternatively, we can choose to not regard it as a bug, and instead say the test is invalid. I do not know the rationale here, but given that it was explicitly said to be intended to work this way, I am on the side of seeing it as a pre-existing bug. One that is nearly impossible to fix in the current structure.

Now that there can only be one valid data layout string per target, if it is intended that `UpgradeDataLayoutString` only upgrade target-valid data layout strings, it is a bug for `UpgradeDataLayoutString` to ever produce anything other than 1) its input or 2) the target's one valid data layout string. This allows a much simpler implementation that completely fixes the bug, but is too big to be part of this change. I would like to propose that in this change, we change `UpgradeDataLayoutString` to insert `-i128:128-` including in that one test, and we XFAIL `upgrade-datalayout2.ll` since the uncovered bug is not actually a new bug. In a followup PR, I can then restructure the `UpgradeDataLayoutString` logic by removing the function entirely and instead having target functions to check whether a given data layout string is a valid historic data layout string for the target that should be upgraded, and if so, simply clobbering the data layout string with what the target reports is the correct data layout string.

Does that seem reasonable? Am I overlooking anything that would make this a non-option? Are there good alternatives that I am not seeing right now?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86310/new/

https://reviews.llvm.org/D86310