Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TargetData needs to be parameterized better in the .bc/.ll files #1133

Closed
lattner opened this issue May 2, 2006 · 12 comments
Closed

TargetData needs to be parameterized better in the .bc/.ll files #1133

lattner opened this issue May 2, 2006 · 12 comments
Labels
bugzilla Issues migrated from bugzilla enhancement Improving things as opposed to bug fixing, e.g. new or missing feature portability

Comments

@lattner
Copy link
Collaborator

lattner commented May 2, 2006

Bugzilla Link 761
Resolution FIXED
Resolved on Mar 06, 2010 14:00
Version 1.0
OS All
Depends On #1017
Blocks llvm/llvm-bugzilla-archive#762

Extended Description

Currently, when 'opt' is run, it configures and adds a TargetData to the pass manager based on the
settings of the "target" flags in the Module. These flags indicate whether the target is 32/64 bit, and
whether it is big or little endian.

This has two problems:

  1. If the module has no target info, we default to SparcV9 (this is Bug 760).
  2. Our mapping from ptrsize/endianness to a full targetdata is a total hack.

#​2 is true because a TargetData has many more fields than just ptrsize and endianness, thus the
mapping from these two fields to a full TargetData is inherently a few-to-many mapping, which is
impossible to be right in the general case (we have it carefully rigged to work for our current targets
though).

Instead of encoding ptrsize/endianness into the module, it would be far better to encode a whole target
data into it. I propose that we do this defining a "default" target data, then encoding any diffs from this
into the module as a string. For example, if the default was a 64-bit, big endian, target with 8-byte
aligned doubles, a target like X86 could be described as "p3eLd4".

As TargetData is extended in the future, more letters could be gracefully added without breaking
old .bc files.

-Chris

@lattner
Copy link
Collaborator Author

lattner commented May 2, 2006

Oh yeah, TargetData should have a ctor that takes the string form, and should have a method that
produces it from a given TargetData.

Among other things (e.g. correctness on new targets), this would allow (for example) the TargetMachine
ctors to pass in a short constant string to their TargetData, instead of passing 50 random options.

-Chris

@lattner
Copy link
Collaborator Author

lattner commented Jan 18, 2007

Owen, is this done or nearly done?

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 19, 2007

After mistakenly trying to implement bc read/write for this, I had to back it
out because there's a lot more work to be done:

  1. The bcwriter needs to get rid of calls to getEndianness and getPointerSize
    and not set that information in the module header. Instead it should call
    getDataLayout and write that string.

  2. The reader needs to be updated to get rid of calls to setEndianness and
    setPointerSize and ignore the bits in the module header for them. Instead it
    needs to setDataLayout.

  3. The parsing routines in Module.cpp for this stuff are not very clean nor
    optimal. In particular, setEndianness appends to the end of what it assumes
    is an empty string. THis isn't true after bcreader calls setDataLayout.

  4. The Interpreter.cpp needs to compute the data layout for the machine its
    running on and call setDataLayout instead of setEndianness and setPointerSize

  5. CloneModule needs to copy dataLayout, not endianness/pointer size.

  6. LinkModules needs to deal with this correctly too.

  7. There may be other stuff too.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 19, 2007

One more thing:

The setDataLayout function should be changed like this:

  • void setDataLayout(std::string DL) { DataLayout = DL; }
  • void setDataLayout(const std::string& DL) { DataLayout = DL; }

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 23, 2007

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 23, 2007

Patch for llvm-gcc4

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 23, 2007

Patch for testsuite

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 23, 2007

I've attached the necessary patches for this bug. These can't go in, however, until #1017 is resolved so
that support for upgrading can go into the new llvm-upgrade.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 26, 2007

This has now been implemented. The patches for the code have been applied. The
patch for llvm-gcc4 now needs to be committed. The patch for the testsuite was
partially applied. In some cases it was preferable to leave the old
endian/pointersize specifications in and let llvm-upgrade upgrade them.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 26, 2007

Owen,

You're still on the hook for the bytecode format changes in the documentation.

Reid.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 27, 2007

@lattner
Copy link
Collaborator Author

lattner commented Nov 27, 2021

mentioned in issue llvm/llvm-bugzilla-archive#762

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 3, 2021
@Endilll Endilll added enhancement Improving things as opposed to bug fixing, e.g. new or missing feature and removed new-feature labels Aug 15, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla enhancement Improving things as opposed to bug fixing, e.g. new or missing feature portability
Projects
None yet
Development

No branches or pull requests

3 participants