Page MenuHomePhabricator

For RenderScript, set alignment and width of long to 64-bits
AbandonedPublic

Authored by pirama on May 26 2016, 4:01 PM.

Details

Summary

This patch sets width and alignment of 'long' to 64-bits if the
Renderscript LangOpt (also added as part of this patch) is set.

This patch also makes the LangOpt visible by adding 'renderscript' as a
new language type that can be specified as an option to '-x'.

Diff Detail

Event Timeline

pirama updated this revision to Diff 58713.May 26 2016, 4:01 PM
pirama retitled this revision from to Support ARM subtarget feature +long64.
pirama updated this object.
pirama added a reviewer: kristof.beyls.
pirama added subscribers: srhines, cfe-commits.
kristof.beyls edited edge metadata.May 30 2016, 4:49 AM

Hi Pirama,

My understanding is that this introduces (yet another) ARM 32 bit ABI variant - in this case with longs being 64 bit.
My understanding is also that this is the ABI that is used in Renderscript, and this patch helps to remove local patches that currently live in the Renderscript toolchain downstream only.

I think it's good not to need downstream patches for a Renderscript toolchain.

In effect, this introduces another abi variant.
I'm wondering if the abi variant should be controlled using a triple, just like e.g. the hard float vs. soft float abi variants also get controlled via a triple?
Tim, do you happen to have insights on this?

Thanks,

Kristof

Hi Pirama,

My understanding is that this introduces (yet another) ARM 32 bit ABI variant - in this case with longs being 64 bit.
My understanding is also that this is the ABI that is used in Renderscript, and this patch helps to remove local patches that currently live in the Renderscript toolchain downstream only.

Correct. This is only used by RenderScript, and unfortunately can't be done any differently. We had hoped to just predicate this with our own LangOpt (the patch that adds the RenderScript LangOpt is coming soon), but unfortunately, at the point where LangOpts are being parsed, the target machine already has the size and alignment of long specified (and it can't change).

I think it's good not to need downstream patches for a Renderscript toolchain.

Same here, although these patches seem so small that I don't want to introduce too much trouble in upstream. If there isn't a nice way to clean this up, I would be fine with this amount of divergence, since it really doesn't impact our ability to update LLVM (and again, the feature is only used by RenderScript).

In effect, this introduces another abi variant.
I'm wondering if the abi variant should be controlled using a triple, just like e.g. the hard float vs. soft float abi variants also get controlled via a triple?

We can't unfortunately control this via a separate triple because RenderScript has (and always) will use the ARM triple. This really only affects the frontend for RenderScript, since RS doesn't allow the full libc runtime. That is why the LLVM side of this patch has no tests (because there is nothing there, and we only need the option recognized or the backend generates warnings about an unsupported flag - perhaps there is a better way to suppress that, but at the time that I fixed this years ago, I think my only option was to add it in both places).

Tim, do you happen to have insights on this?

Thanks,

Kristof

Correct. This is only used by RenderScript, and unfortunately can't be done any differently. We had hoped to just predicate this with our own LangOpt (the patch that adds the RenderScript LangOpt is coming soon), but unfortunately, at the point where LangOpts are being parsed, the target machine already has the size and alignment of long specified (and it can't change).

Hi Stephen,

GCC uses -mabi=ilp32 or -mabi=lp64 for AArch64 to change the ABI, you could do the same on Renderscript.

If this is how Renderscript *always* operate, than having a "renderscript" ABI would make even more sense.

cheers,
--renato

I looked at how LangOpts and TargetInfo are handled and found a way we can achieve 64-bit longs without a Target feature (and restrict it just to RenderScript!). This would be via TargetInfo::adjust(). I'll abandon the sibling patch to LLVM as we should be able to do this just in Clang.

Adding Richard to reviewers as the planned direction of this patch directly relates to the Frontend.

pirama updated this revision to Diff 59294.Jun 1 2016, 3:42 PM
pirama removed a reviewer: rsmith.

Add a RenderScript langopt and updated to change long's size and alignment in
TargetInfo::adjust().

rsmith added a subscriber: rsmith.Jun 1 2016, 3:52 PM

The summary and content of this patch are very different.

If you have an ABI variant that any language should be able to target, that should be controlled by the triple and related flags, not by a RenderScript language option.

If you want to introduce RenderScript as a supported language in Clang, that may well be reasonable, but that's a very different discussion.

Normally, we don't want to have ABI variants in clang that only a single language can target. There doesn't seem to be any fundamental reason why you couldn't compile C++ code for the RenderScript ABI, for instance, so tying the ABI to the language doesn't seem like the right approach.

pirama retitled this revision from Support ARM subtarget feature +long64 to For RenderScript, set alignment and width of long to 64-bits.Jun 1 2016, 4:09 PM
pirama updated this object.

After talking to Richard this afternoon on IRC, we are going with a slightly different plan here. We will create two new triples (renderscript32-*-* and renderscript64-*-*), which are direct subclasses of the general Android ARM32 and ARM64 targets. In this case, we can use all of the existing ABI info we already have built up for these targets (and override longs to be 64-bit on the renderscript32-*-* target). Additionally, the renderscript targets will also set the triple name back to the ARM-related triple, since we can't break compatibility for generating RenderScript code that is expected to embed the ARM triples in the bitcode (mostly ignored, but might be validated).

One thing that didn't come up in the IRC discussion, but we probably still need a LangOpt for RenderScript. There is a coming patch that adds a single attribute (for "kernel"), that has error checking to ensure that it is only used by our RenderScript targets. I assume that the cleanest way to retain that diagnostic (and get the last of these divergent patches in) will be to still key off of a LangOpt. If you think it is preferable to check against a target instead, I can change the code to do that, but I figured this is more consistent with how other attributes work.

rsmith added a comment.Jun 7 2016, 6:22 PM

There is a coming patch that adds a single attribute (for "kernel"), that has error checking to ensure that it is only used by our RenderScript targets. I assume that the cleanest way to retain that diagnostic (and get the last of these divergent patches in) will be to still key off of a LangOpt. If you think it is preferable to check against a target instead, I can change the code to do that, but I figured this is more consistent with how other attributes work.

We discussed this again today, and our preferred way forward is:

  1. We have new targets for 32-bit and 64-bit renderscript as described here. The new targets are exactly the same as the relevant ARM targets except that they force the size and alignment of long to 8. (We may want to consider also making these targets imply -emit-llvm, especially if our notional model for the target is that its "machine code" is LLVM IR with an ARM triple.)
  1. We have a new LangOption and InputKind for renderscript, which can be specified via -x renderscript and is implied by a .rs file extension. This LangOption controls the existence of the "kernel" attribute, and some renderscript-specific implicit conversion rules.

... and we treat (1) and (2) as orthogonal concerns (much like the SPIR target and OpenCL language); you can compile plain C or C++ to the renderscript target if you want, or request the renderscript language rules while targeting x86, even though these are not likely to be particularly useful configurations in practice.

D21333 and D21334 add the RenderScript triple and modify 'long' based off the triple.

r272317, r272342, r272438 take care of the LangOpt and handling of '-x renderscript' and '.rs' extensions.

pirama abandoned this revision.Jun 21 2016, 2:06 PM

Abandoning this revision. Patches for an alternative approach are listed above.