Page MenuHomePhabricator

[clang][MIPS] Removing __SIZEOF_INT128__ macro for MIPS64
ClosedPublic

Authored by slthakur on Nov 25 2014, 2:57 AM.

Details

Summary

SIZEOF_INT128 macro should not be defined as 128-bit operations are not yet supported for MIPS64.

Failing tests:
  Lexer/ms-extensions.c:

Test assumes if SIZEOF_INT128 is not defined then use of suffix i128 should throw an error message "int128 is not supported on this target". In MIPS, declaration of int128 type is necessary even though SIZEOF_INT128 is undefined because standard header files like limits.h throw error message if __int128 is not available.

Diff Detail

Repository
rL LLVM

Event Timeline

slthakur updated this revision to Diff 16602.Nov 25 2014, 2:57 AM
slthakur retitled this revision from to [clang][MIPS] Removing __SIZEOF_INT128__ macro for MIPS64.
slthakur updated this object.
slthakur edited the test plan for this revision. (Show Details)
slthakur added reviewers: kcc, samsonov, atanasyan, dsanders.
slthakur set the repository for this revision to rL LLVM.
slthakur added subscribers: sdkie, mohit.bhakkad, Anand.Takale, Unknown Object (MLST).
atanasyan edited edge metadata.Nov 25 2014, 3:10 AM

Could you add a test case checks that the __SIZEOF_INT128__ macro is not defined?

slthakur updated this revision to Diff 16715.Nov 28 2014, 12:58 AM
slthakur updated this object.
slthakur edited edge metadata.
slthakur set the repository for this revision to rL LLVM.

The previous was not allowing declaration of int128 types in standard
header files like limit.h.
This patch will allow declaration of
int128 type and just undefined the 128-bit macro
for mips64 and mips64el.

slthakur updated this object.Nov 28 2014, 1:01 AM
slthakur set the repository for this revision to rL LLVM.
atanasyan accepted this revision.Nov 28 2014, 2:14 AM
atanasyan edited edge metadata.

LGTM with the only nit.

lib/Frontend/InitPreprocessor.cpp
665 ↗(On Diff #16715)

I would add a comment describes that it is a temporary workaround while MIPS has no fully supported 128-bit integers and we do not want to get an error from limit.h.

This revision is now accepted and ready to land.Nov 28 2014, 2:14 AM
slthakur added inline comments.Dec 2 2014, 2:10 AM
lib/Frontend/InitPreprocessor.cpp
665 ↗(On Diff #16715)

Hi @atanasyan,

Should I add a comment and submit a new patch ?

atanasyan added inline comments.Dec 2 2014, 2:21 AM
lib/Frontend/InitPreprocessor.cpp
665 ↗(On Diff #16715)

I think it is not necessary now, because this patch has been committed already.

slthakur added inline comments.Dec 3 2014, 5:00 AM
lib/Frontend/InitPreprocessor.cpp
665 ↗(On Diff #16715)

Hi @atanasyan,

I don't see this patch commited in the clang repository ?

atanasyan added inline comments.Dec 3 2014, 5:37 AM
lib/Frontend/InitPreprocessor.cpp
665 ↗(On Diff #16715)

My bad, I interpreted your question incorrectly. Add the comment to the patch and commit it. It is not necessary to send the patch for review again.

majnemer added inline comments.
lib/Frontend/InitPreprocessor.cpp
665 ↗(On Diff #16715)

Wait, why not just override Mips64TargetInfoBase:: hasInt128Type to be false?

slthakur added inline comments.Dec 4 2014, 12:33 AM
lib/Frontend/InitPreprocessor.cpp
665 ↗(On Diff #16715)

@majnemer : When we override Mips64TargetInfoBase:: hasInt128Type to be false, we get an error message "int128 is not supported on this target" from standard header files like limits.h because these files are using __int128 type and suffix i128. Here, we only want to remove __SIZEOF_INT128__ macro for mips64 and misp64el because 128-bit operations are not yet supported on MIPS64. This change is temporary, until we implement 128-bit operations for MIPS64.

slthakur updated this revision to Diff 16913.Dec 4 2014, 1:00 AM
slthakur edited edge metadata.

Added comment

Hi @atanasyan,

I don't have commit access. Can you commit it for me ?

I was able to #include <limits.h> with just the following diff:

diff --git a/lib/Basic/Targets.cpp b/lib/Basic/Targets.cpp
index b28808b..67bff45 100644

  • a/lib/Basic/Targets.cpp

+++ b/lib/Basic/Targets.cpp
@@ -5971,7 +5971,7 @@ public:

  NumAliases = llvm::array_lengthof(GCCRegAliases);
}
  • bool hasInt128Type() const override { return true; }

+ bool hasInt128Type() const override { return false; }
};

class Mips64EBTargetInfo : public Mips64TargetInfoBase {

I tested it with: echo '#include <limits.h>' |
~/llvm/Debug+Asserts/bin/clang -x c -target mips64-none-none - -fsyntax-only

Hi @majnemer,

Sorry, my mistake. The problem arises when we #include c++ header file <limits> and not with c header file <limits.h>

I tested it with: echo '#include <limits>' | clang++ -x c++ -target mips64-linux-gnu - -fsyntax-only.

I will change the comment.

slthakur updated this revision to Diff 16920.Dec 4 2014, 4:17 AM
slthakur set the repository for this revision to rL LLVM.

Changed comment in /lib/Frontend/InitPreprocessor.cpp

This revision was automatically updated to reflect the committed changes.
dsanders edited edge metadata.Dec 11 2014, 3:06 AM

Hi,

I stumbled across the remaining part of the backend problem while investigating some failures on a mips64 host and mips64-linux-gnu triple. http://reviews.llvm.org/D6610 should fix it.