vector data types support in the LLVM interpreter part 2

Nick Lewycky nicholas at mxc.ca
Fri May 3 10:45:39 PDT 2013


I'm interested in deleting the llvm intepreter entirely. Are you using 
it? Why?

+#define IMPLEMENT_VECTOR_INTEGER_ICMP(OP, TY)                        \
+  case Type::VectorTyID: {                                           \
+    assert(Src1.AggregateVal.size() == Src2.AggregateVal.size());    \
+    Dest.AggregateVal.resize( Src1.AggregateVal.size() );            \
+    for( uint32_t _i=0;_i<Src1.AggregateVal.size();_i++)             \
+      Dest.AggregateVal[_i].IntVal = APInt(1,                        \
+      Src1.AggregateVal[_i].IntVal.OP(Src2.AggregateVal[_i].IntVal));\
+  } break;

Don't start variable names with underscores. Seriously, just using 'i' 
should be fine here, it won't shadow due to the braces around the whole 
case. See 
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Don't re-evaluate Src1.AggregateVal.size() each time through the loop, 
hoist it out into an end variable. See 
http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop

Don't put spaces inside the parentheses for resize() and for(). See the 
examples in 
http://llvm.org/docs/CodingStandards.html#microscopic-details or just 
clang-format it.

+#define IMPLEMENT_VECTOR_FCMP(OP)                                   \
+  case Type::VectorTyID:                                            \
+    if(dyn_cast<VectorType>(Ty)->getElementType()->isFloatTy()) {   \
+      IMPLEMENT_VECTOR_FCMP_T(OP, Float);                           \
+    } else {                                                        \
+        IMPLEMENT_VECTOR_FCMP_T(OP, Double);                        \
+    }

Use dyn_cast if you don't know the type and can tolerate receiving a 
NULL. In this case, you checked for TY->isVectorTy() and therefore 
should just use cast<VectorType> instead of dyn_cast. See 
http://llvm.org/docs/ProgrammersManual.html#isa

Also, what's up with the different amount of leading space on the 
opposite sides of the if/else? Please use two-spaces consistently. Also, 
you're missing a space after the 'if' keyword.

+        if (DestMask.AggregateVal[_i].IntVal == true)                    \

if (x == true) is redundant.

  static GenericValue executeFCMP_UEQ(GenericValue Src1, GenericValue Src2,
                                     Type *Ty) {
    GenericValue Dest;
    IMPLEMENT_UNORDERED(Ty, Src1, Src2)
+  MASK_VECTOR_NANS(Ty, Src1, Src2, true)
+  IMPLEMENT_VECTOR_UNORDERED(Ty, Src1, Src2, executeFCMP_OEQ)
    return executeFCMP_OEQ(Src1, Src2, Ty);
+
  }

Please remove the extra blank line at the end of the function.

+        Dest.AggregateVal[_i].IntVal = APInt(1,
+        ( (Src1.AggregateVal[_i].DoubleVal ==
+        Src1.AggregateVal[_i].DoubleVal) &&
+        (Src2.AggregateVal[_i].DoubleVal ==
+        Src2.AggregateVal[_i].DoubleVal)));

Run clang-format over your whole patch. It lives in the clang repository 
under tools/clang-format and should be built if you do a normal 
llvm+clang build. See 
http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting for 
documentation on how to format a patch without formatting the untouched 
parts of the file.

Nick

Veselov, Yuri wrote:
> Sorry, now with attachment
>
> *From:*Veselov, Yuri
> *Sent:* Monday, April 15, 2013 9:24 PM
> *To:* Nadav Rotem; llvm-commits at cs.uiuc.edu
> *Subject:* RE: vector data types support in the LLVM interpreter part 2
>
> Hi
>
> This is the second part of code which implements vector support for LLVM
> interpreter.
>
> It implements binary and relational operations for integer and floating
> point data types.
>
> The first part was committed April,01,2013, rev. 178469.
>
> Thanks,
>
> Yuri
>
> *From:*Nadav Rotem [mailto:nrotem at apple.com]
> *Sent:* Monday, April 15, 2013 9:15 PM
> *To:* Veselov, Yuri
> *Subject:* Re: vector data types support in the LLVM interpreter part 2
>
> Can you please send the patch to the mailing list ?
>
> On Apr 15, 2013, at 10:01 AM, "Veselov, Yuri" <Yuri.Veselov at intel.com
> <mailto:Yuri.Veselov at intel.com>> wrote:
>
> I’m sorry for inconvenience,
>
> Before send the patch, I have built the patched code on SLES and win 7,
> and it was built successfully, but it looks like it was not enough.
>
> Is it possible for me to pass any kind of pre-commit ?
>
> So, I send you fixed patch (attached).
>
> Thanks,
>
> Yuri
>
> *From:*Nadav Rotem [mailto:nrotem at apple.com]
> *Sent:*Saturday, April 13, 2013 2:05 AM
> *To:*Veselov, Yuri
> *Subject:*Re: vector data types support in the LLVM interpreter part 2
>
> Yuri,
>
> I committed it r179409 but I had to revert it because it broke the
> build. Please send the revised patch to llvm-commit.
>
> http://lab.llvm.org:8011/builders/lld-x86_64-darwin11/builds/757
>
> Thanks,
>
> Nadav
>
> On Apr 12, 2013, at 12:36 AM, "Veselov, Yuri" <Yuri.Veselov at intel.com
> <mailto:Yuri.Veselov at intel.com>> wrote:
>
>
>
> Ok, thanks a lot.
>
> -Yuri
>
> *From:*Nadav Rotem [mailto:nrotem at apple.com]
> *Sent:*Thursday, April 11, 2013 9:18 PM
> *To:*Veselov, Yuri
> *Subject:*Re: vector data types support in the LLVM interpreter part 2
>
> I think that its okay. I didn't get a chance to commit it yes, but the
> patch is on my desktop. I hope to get it in today.
>
> Thanks,
>
> Nadav
>
> On Apr 11, 2013, at 10:09 AM, "Veselov, Yuri" <Yuri.Veselov at intel.com
> <mailto:Yuri.Veselov at intel.com>> wrote:
>
>
>
>
> Hi Nadav,
>
> Is anything wrong with patch ? Should I change something?
>
> Thanks,
>
> Yuri
>
> *From:*Nadav Rotem [mailto:nrotem at apple.com]
> *Sent:*Monday, April 08, 2013 9:22 PM
> *To:*Veselov, Yuri
> *Subject:*Re: vector data types support in the LLVM interpreter part 2
>
> LGTM. I will commit this later today.
>
> On Apr 8, 2013, at 10:02 AM, "Veselov, Yuri" <Yuri.Veselov at intel.com
> <mailto:Yuri.Veselov at intel.com>> wrote:
>
>
>
>
>
> Hi Nadav,
>
> Could you please review the second patch implementing vector data
> support to LLVM interpreter?
>
> *Best regards,*
>
> *Yuri Veselov*
>
> INNL, Nizhny Novgorod, Russia
>
> '/+7(831)/*296**-9**4**-**44**;*/ext./*2307*
>
> ***Yuri.Veselov at Intel.com <mailto:Yuri.Veselov at Intel.com>
>
>
> --------------------------------------------------------------------
> Closed Joint Stock Company Intel A/O
> Registered legal address: Krylatsky Hills Business Park,
> 17 Krylatskaya Str., Bldg 4, Moscow 121614,
> Russian Federation
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
> <interpreter_patch_2.patch>
>
>
> --------------------------------------------------------------------
> Closed Joint Stock Company Intel A/O
> Registered legal address: Krylatsky Hills Business Park,
> 17 Krylatskaya Str., Bldg 4, Moscow 121614,
> Russian Federation
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
>
> --------------------------------------------------------------------
> Closed Joint Stock Company Intel A/O
> Registered legal address: Krylatsky Hills Business Park,
> 17 Krylatskaya Str., Bldg 4, Moscow 121614,
> Russian Federation
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
>
> --------------------------------------------------------------------
> Closed Joint Stock Company Intel A/O
> Registered legal address: Krylatsky Hills Business Park,
> 17 Krylatskaya Str., Bldg 4, Moscow 121614,
> Russian Federation
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
> <interpreter_patch_2_2.patch>
>
>
> --------------------------------------------------------------------
> Closed Joint Stock Company Intel A/O
> Registered legal address: Krylatsky Hills Business Park,
> 17 Krylatskaya Str., Bldg 4, Moscow 121614,
> Russian Federation
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits




More information about the llvm-commits mailing list