[PATCH] D44426: Fix llvm + clang build with Intel compiler

[Melanie Blower via Phabricator via cfe-commits]

mibintc added inline comments.


================
Comment at: include/llvm-c/Target.h:25
 
-#if defined(_MSC_VER) && !defined(inline)
+#if defined(_MSC_VER) && !defined(inline) && !defined(__INTEL_COMPILER)
 #define inline __inline
----------------
yvvan wrote:
> mibintc wrote:
> > I really think all the Intel-compiler bug workarounds should be version specific. For example, in the boost.org customizations we have checks like this:
> > #if defined(__INTEL_COMPILER) && (__INTEL_COMPILER >= 1500)
> > 
> > I believe you are using the most recent Intel compiler, 18.0, which has version 1800.  Let's assume the bug will either be fixed in our 18.0 compiler or in our next compiler which would be numbered 1900, so the check could be like this:
> > #if defined(__INTEL_COMPILER) && (__INTEL_COMPILER < 1900) // employ the workaround for the Intel 18.0 compiler and older
> > 
> > This way the workaround will "disappear" when the bug gets fixed.
> > 
> > For that matter I wonder if it's still necessary to use the workaround for the Microsoft compiler?
> > 
> > Thanks for reporting the bug into the Intel forum. If we put a bug workaround into LLVM it would be very nice to have the bug reported to the offending compiler. 
> > 
> This one will probably not be fixed because I was answered that it's the predicted behavior and that I need not to "#define inline __inline" if I use intel compiler
> I agree though about versions which need to be taken into account
thanks


================
Comment at: include/llvm/ADT/BitmaskEnum.h:73
 
+#ifdef __INTEL_COMPILER
+template <typename E>
----------------
yvvan wrote:
> mibintc wrote:
> > what problem is being worked around here? I checked your post into the Intel compiler forum "enum members are not visible to template specialization" and I can observe the bug on our Windows compiler but not our Linux compiler. 
> it was recently accepted. the issues is that E::LLVM_BITMASK_LARGEST_ENUMERATOR from line 80 here is always invalid with intel compiler and the whole set of operator overloads is ignored
> at least on Windows :)
Thanks. I found the bug report for this one in the icc bug tracking database. It's CMPLRS-47898. With luck it will be fixed in the next update (ballpark based on previous release cadence, 3 months)


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:254
   FMRB_OnlyAccessesInaccessibleOrArgMem = FMRL_InaccessibleMem |
-                                          FMRL_ArgumentPointees |
+                                          static_cast<int>(FMRL_ArgumentPointees) |
                                           static_cast<int>(ModRefInfo::ModRef),
----------------
yvvan wrote:
> mibintc wrote:
> > is this a necessary workaround for the Intel compiler? It's not pretty.  Suggest we add the #if around it? Long term i hope this would not be necessary.
> i'm not sure that adding #if around makes it easier to understand :)
> of course I can add it everywhere I append enums with static_cast<int>
The modification for the Intel compiler is so ugly that I'd rather it get wrapped with the #if, and eventually the workaround could get discarded. Imagine yourself 20 years from now staring at that code and wondering why it is written that way.  The #if makes it stand out, and easier to find later, and easier to restore to pristine state, when you want to clean it up.  

I tried a small test case with the Intel compiler but i couldn't see the problem, must be too simple-minded.
typedef enum { a,b,c,d,e } letters;
char conv( letters l) {
  switch(l) {
  case a: return 'a';
  case a | b : return 'b';
  default : return 'z';
  }
}



================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:6131
 
+#ifndef __INTEL_COMPILER
         static_assert(((~(SIInstrFlags::S_NAN |
----------------
yvvan wrote:
> mibintc wrote:
> > this assesrts with Intel compiler?  Or the expression needs to be rewritten with static_cast as above?
> "needs to be rewritten with static_cast" - correct. it can be done but I was a bit lazy and statioc_assert does not affect the outcome of build :)

ok, it's got the if intel so: lgtm


https://reviews.llvm.org/D44426