Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[mlir][LLVM] Purge uses of LowerToLLVMOptions::datalayout that should be replaced by getAnalysis<DataLayoutAnalysis>()
Needs ReviewPublic

Authored by nicolasvasilache on Aug 11 2023, 8:23 AM.



FuncToLLVM uses the data layout string attribute in 3 different ways:

  1. LowerToLLVMOptions options(&getContext(), getAnalysis<DataLayoutAnalysis>().getAtOrAbove(m));
  2. options.dataLayout = llvm::DataLayout(this->dataLayout);
  3. m->setAttr(..., this->dataLayout));

This revision shows that 2. is basically unused:

  1. the patterns populated from FuncToLLVM.cpp do not use options.dataLayout
  2. the only patterns using options.dataLayout are in ConvertVectorToLLVM but the only way they are called is with the default data layout string ""

Given the inter-pass consistency issues raised in this post,
this is another latent consistency footgun that we can nip in the bud.

In the future, we need to standardize on MLIR DataLayoutAnalysis but the cleanup of unused LowerToLLVMOptions::dataLayout / its constant propagation to
its users, reduces chances for errors.

  1. is retired in a separate PR

In the future, we should all converge on 1. but this is beyond the scope of this revision.

Diff Detail

Event Timeline

Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
nicolasvasilache requested review of this revision.Aug 11 2023, 8:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2023, 8:23 AM
mehdi_amini added inline comments.Aug 29 2023, 3:25 PM

There should be some sort of TODO here?


What's the plan here in the future? I didn't quite follow from the patch description?

nicolasvasilache marked an inline comment as done.
nicolasvasilache edited the summary of this revision. (Show Details)

Rebase and add comments.

nicolasvasilache marked an inline comment as done.

Update commit message


Spelled it out a little more in the description: we'll need to encode the proper information using MLIR's DataLayoutAnalysis and make use of it.
In the meantime, what I did here is a "constant propagation": the implementation gave the wrong impression that
typeConverter.getDataLayout() ~= LowerToLLVMOptions::dataLayout was used, in practice it is not so let's remove that footgun until we really support DataLayout.