This is an archive of the discontinued LLVM Phabricator instance.

DataLayout is mandatory, update the API to reflect it with references.
ClosedPublic

Authored by mehdi_amini on Mar 8 2015, 7:40 PM.

Details

Reviewers
echristo
Summary

Now that the DataLayout is a mandatory part of the module, let's start
cleaning the codebase. This patch is a first attempt at doing that.

This patch is not exactly NFC as for instance some places were passing a
nullptr instead of the DataLayout, possibly just because there was a
default value on the DataLayout argument to many functions in the API.
Even though it is not purely NFC, there is no change in the validation.

I turned as many pointer to DataLayout to references, this helped figuring
out all the places where a nullptr could come up.

I had initially a local version of this patch broken into over 30 independant,
commits but some later commit were cleaning the API and touching part of the
code modified in the previous commits, so the intermediate steps are not very
clean. Here is the squashed result.

Diff Detail

Repository
rL LLVM

Event Timeline

mehdi_amini updated this revision to Diff 21469.Mar 8 2015, 7:40 PM
mehdi_amini retitled this revision from to DataLayout is mandatory, update the API to reflect it with references. .
mehdi_amini updated this object.
mehdi_amini edited the test plan for this revision. (Show Details)
mehdi_amini added a reviewer: echristo.
mehdi_amini set the repository for this revision to rL LLVM.
mehdi_amini added a subscriber: Unknown Object (MLST).
echristo accepted this revision.Mar 9 2015, 10:31 AM
echristo edited edge metadata.

Couple of comments, in general looks fine and anything else can be covered in post-commit etc based on answers.

You pass down the DataLayout in a few new places rather than keep it cached or look it up, any reason? (This is also one of the inline comments as I found it in a particular place, but rather than keep asking it's also a top level comment).

Thanks for the work! That was very long :)

-eric

lib/Analysis/Lint.cpp
178

Any reason not to leave it cached instead of passing it down?

lib/Analysis/LoopAccessAnalysis.cpp
291–292

Formatting. :)

This revision is now accepted and ready to land.Mar 9 2015, 10:31 AM

Thanks for review, that was quick :)

lib/Analysis/Lint.cpp
178

In short: cannot rebind a reference and I wanted to get rid of the pointer.

It plays well with passes that initialize a separate transient object to do the processing, but not with passes that do their processing "in-place". Because passes object are not transient, they carry their state between runs, including a potential obsolete DL pointer.

Not sure if I was clear?

This was very case-by-case, and I may have get it wrong here and there (I mean wrong in the sense that a function argument could have been save by retrieving the DL differently, I don't think I broke correctness).

lib/Analysis/LoopAccessAnalysis.cpp
291–292

That was clang-format, it probably gets over 80 lines without the line break. I'll rephrase the comment to save what is needed to fit in the line.

mehdi_amini updated this revision to Diff 21534.Mar 9 2015, 5:46 PM
mehdi_amini edited edge metadata.
  • add a DataLayout parameter to getOrEnforceKnownAlignment() instead of using CtxI instruction to please clang (which does not have an instruction hanging around).
  • cleanup:
    • some includes were in header where they could have been in .cpp.
    • rename TD to DL everywhere for consistency (DataLayout was TargetData in the past apparently).
    • fixup all comments mentioning what to do in case of missing DataLayout
mehdi_amini closed this revision.Mar 9 2015, 7:42 PM

r231739/r231740