This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, i386, middle-end, tessuite] Intel TSX's HLE.


Stupid mailer.. sigh. trying again:

On 04/18/2012 05:36 AM, Kirill Yukhin wrote:

   op = expand_normal (exp);
-  if (INTVAL (op) < 0 || INTVAL (op) >= MEMMODEL_LAST)
+  if (INTVAL (op) < 0)
     {
       warning (OPT_Winvalid_memory_model,
            "invalid memory model argument to builtin");
       return MEMMODEL_SEQ_CST;
     }
+
   return (enum memmodel) INTVAL (op);
 }

I think this could do better.


It suppose to check that the memory model is one of MEMMODEL_ enum's, and if it isnt' report the error and change it to SEQ_CST. This is to prevent people from accidentally passing invalid numbers in, like say '23'
I think this ought to check that
(INTVAL(op) & memmodel_mask) is within the proper range.


It would be better if we could check that only other valid recognized bits are set in the other word, but Im guessing thats not really possible since thats really target specific at the moment.

In any case, I think it ought to check that the 16 reserved bits for memory model is correct (like it use to for the whole enum), and if it isn't, issue the warning and mask in SEQ_CST for the memory model portion.

I also think that it should no longer return an 'enum memmodel' since the value can now be out of range. Maybe thats more a nit... how much do we care if we use an enum value as an int? ie, do we need to worry about passing this value around as an 'enum memodel' when we know its more than 16 bits sometimes now?

Andrew






Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]