This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add initial support for -meabi flag
ClosedPublic

Authored by tinti on Oct 29 2015, 12:43 PM.

Details

Summary

The -meabi flag to control LLVM EABI version.

Without '-meabi' or with '-meabi default' imply LLVM triple default.
With '-meabi gnu' sets EABI GNU.
With '-meabi 4' or '-meabi 5' set EABI version 4 and 5 respectively.

Diff Detail

Event Timeline

tinti updated this revision to Diff 38758.Oct 29 2015, 12:43 PM
tinti retitled this revision from to [clang] Add initial support for -meabi flag.
tinti updated this object.
tinti added reviewers: rengolin, compnerd, zatrazz.
tinti set the repository for this revision to rL LLVM.
tinti added a subscriber: cfe-commits.
tinti added a subscriber: behanw.Oct 29 2015, 2:10 PM
rengolin added subscribers: jroelofs, t.p.northover.

You forgot to add the context. It makes a big difference in the driver code. :)

But overall, looks good to me.

@jroelofs @compnerd @t.p.northover, can you see anything that might impact your workloads?

cheers,
--renato

tinti updated this revision to Diff 38803.Oct 30 2015, 6:31 AM
tinti edited edge metadata.
tinti removed rL LLVM as the repository for this revision.

Add context

compnerd edited edge metadata.Oct 30 2015, 7:21 AM

This looks like what I had in mind wrt use of -meabi.

lib/CodeGen/BackendUtil.cpp
524

I'd really rather see this written in the LLVM Style:

EABI4, EABI5, GNU

since all of them are initialisms.

BTW, please clang-format this change.

lib/Frontend/CompilerInvocation.cpp
460

If llvm::EABI::EABIVersionType had an Invalid value in the enumeration, you could convert directly to the value here, and report the error if the value was Invalid.

tinti added inline comments.Oct 30 2015, 10:24 AM
lib/CodeGen/BackendUtil.cpp
524

Agreed.

lib/Frontend/CompilerInvocation.cpp
460

I chose this way because none of the other target options have it [1].

Do you prefer with it?

[1] https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Target/TargetOptions.h#L27

tinti added inline comments.Oct 30 2015, 10:33 AM
lib/Frontend/CompilerInvocation.cpp
460

Looks like that in LLVM the Invalid is not defined but in Clang there are target specific enums that define them [1].

[1] https://github.com/llvm-mirror/clang/blob/master/lib/Driver/Tools.h#L263

rengolin added inline comments.Nov 3 2015, 4:53 AM
lib/Frontend/CompilerInvocation.cpp
460

@compnerd, it seems that this code is already checking for invalid values, I'm not sure what you're asking about.

Also, EABI::EABIVersionType has no field Invalid, and I don't think it should, either.

tinti updated this revision to Diff 39108.Nov 3 2015, 1:08 PM
tinti edited edge metadata.
tinti set the repository for this revision to rL LLVM.
  • Clang format code.
  • Update eabi names to match new LLVM patch.
tinti updated this revision to Diff 39304.Nov 4 2015, 6:24 PM

Clang format.

tinti marked 2 inline comments as done.Nov 4 2015, 6:27 PM
tinti added inline comments.
lib/Frontend/CompilerInvocation.cpp
18

This part of the code does not include TargetOptions.h (so EABI is an undefined type). Can I add it here?

rengolin added inline comments.Nov 5 2015, 12:50 PM
lib/Driver/Tools.cpp
3415

Shouldn't we only add the option if it was used in the command line?

lib/Frontend/CompilerInvocation.cpp
18

If you mean up there, at the beginning of the file, then I suspect you should. Though, I though some other header would get it by now. If they don't, then please add it.

tinti updated this revision to Diff 39579.Nov 6 2015, 12:59 PM
tinti removed rL LLVM as the repository for this revision.
tinti updated this revision to Diff 39596.Nov 6 2015, 2:42 PM
tinti set the repository for this revision to rL LLVM.
tinti marked an inline comment as done.
  • Add test for error check
  • Change StringSwitch to use lllvm::EABI type
tinti marked 6 inline comments as done.Nov 6 2015, 2:55 PM
tinti added inline comments.
lib/Driver/Tools.cpp
3415

Good point! Fixed.

lib/Frontend/CompilerInvocation.cpp
460

I have added. It requires to add an invalid or more apropriate name Unknown version.

If the backend ever sees an Unknown it will consider it as a Default.
The frontend considers Unknown as an invalid state and raises an error.

rengolin accepted this revision.Nov 9 2015, 2:55 AM
rengolin edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Nov 9 2015, 2:55 AM
rengolin closed this revision.Nov 9 2015, 4:43 AM

r252463