[llvm-commits] [llvm] r55969 - in /llvm/trunk: docs/LangRef.html include/llvm/Bitcode/LLVMBitCodes.h include/llvm/InstrTypes.h include/llvm/Instructions.h lib/AsmParser/llvmAsmParser.y lib/Bitcode/Reader/BitcodeReader.cpp lib/Bitcode/Writer/BitcodeWriter.cpp lib/VMCore/Verifier.cpp test/Assembler/vbool-cmp.ll test/Assembler/vector-select.ll

[Dan Gohman]

On Sun, September 21, 2008 11:26 pm, Chris Lattner wrote:
>
> On Sep 8, 2008, at 6:02 PM, Dan Gohman wrote:
>
>> Author: djg
>> Date: Mon Sep  8 20:02:47 2008
>> New Revision: 55969
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=55969&view=rev
>> Log:
>> Extend the vcmp/fcmp LLVM IR instructions to take vectors as arguments
>> and, if so, to return a vector of boolean as a result;
>
> Hi Dan and Preston,
>
> I thought the idea here was to change vicmp and vfcmp to have this
> behavior, not change icmp/fcmp to do this.  The problem with changing
> icmp/fcmp is that you'll break a lot of assumptions throughout the
> optimizer that the result of icmp/fcmp is an i1, and the code to
> handle this has already been updated for v*cmp.

Hi Chris,

Overloading plain icmp/fcmp is consistent with how many other
operators are overloaded for vector and scalar. It's a
longer-term goal here is to eventually obviate v*cmp. This
patch is merely a first step, and a prerequisite for much of
the remaining steps.

The optimizers can be made to check for i1 when necessary. This
was included in the vector comparison plan. None of the LLVM
front-ends currently produce vector comparisons, so this doesn't
need to be addressed immediately.

>
>>
>>> The FUNC_CODE_INST_VCMP thing is a little counter-intuitive. I guess
>>> it's necessary in order to remain compatible with exiting vector
>>> FUNC_CODE_INST_CMP usage though, right?
>>
>> Correct. Since FUNC_CODE_INST_CMP with vector arguments is used for
>> the
>> vfcmp and vicmp instructions, it was necessary to add
>> FUNC_CODE_INST_VCMP to support the vector forms of fcmp/icmp. It would
>> probably have made more sense in the bitcode reader/writer to switch
>> vicmp/vfcmp to use a newly defined bitcode operator, but I did not
>> because it seemed likely that it would break existing code.
>
> Is there any reason to keep around the old v*cmp behavior of returning
> a vector of integers?  If not, please remove them (as soon as there
> are no regressions from doing this).  I don't want to have two
> different vector compare implementations floating around.

{i,f}cmp are not yet ready to replace v*cmp. Currently,
codegen cannot effectively handle vectors of i1 on current
targets. The goal is to improve codegen and eventually make
v*cmp unnecessary, but it'll take some time, and until then
we don't want to pull the rug out from people using v*cmp.

> Also, LLVM 2.4 is coming up very soon now, would you be ok with
> deferring (and reverting) these changes until after 2.4 branches in a
> couple weeks?  If you'd prefer to do development on a branch until
> then that would be fine of course.

I view vector {i,f}cmp support as having relatively low risk, since
no front-end or optimizer in tree is producing them. I'd be fine
with adding a disclaimer to LangRef, or even removing them from
LangRef, to help avert misunderstanding.

>> --- llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h (original)
>> +++ llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h Mon Sep  8
>> 20:02:47 2008
>> @@ -205,7 +205,9 @@
>>     // FIXME: Remove GETRESULT in favor of EXTRACTVAL in LLVM 3.0
>>     FUNC_CODE_INST_GETRESULT   = 25, // GETRESULT:  [ty, opval, n]
>>     FUNC_CODE_INST_EXTRACTVAL  = 26, // EXTRACTVAL: [n x operands]
>> -    FUNC_CODE_INST_INSERTVAL   = 27  // INSERTVAL:  [n x operands]
>> +    FUNC_CODE_INST_INSERTVAL   = 27, // INSERTVAL:  [n x operands]
>> +    // fcmp/icmp returning vector of Int1Ty, NOT for vicmp/vfcmp
>> +    FUNC_CODE_INST_VCMP        = 28  // VCMP:       [opty, opval,
>> opval, pred]
>
> Ewww.  I still think that vcmp should change into this behavior, if
> there is some reason why this would not work, please pick a better
> name than INST_VCMP for this!

It's already renamed in TOT :-). Oops, the comment isn't updated,
but the code is. I'll fix that.

>> --- llvm/trunk/include/llvm/InstrTypes.h (original)
>> +++ llvm/trunk/include/llvm/InstrTypes.h Mon Sep  8 20:02:47 2008
>> @@ -18,6 +18,7 @@
>>
>> #include "llvm/Instruction.h"
>> #include "llvm/OperandTraits.h"
>> +#include "llvm/DerivedTypes.h"
>
> Please move the method out of line, to avoid this #include.

Ok.

>> @@ -732,6 +733,13 @@
>>   static inline bool classof(const Value *V) {
>>     return isa<Instruction>(V) && classof(cast<Instruction>(V));
>>   }
>> +  /// @brief Create a result type for fcmp/icmp (but not vicmp/vfcmp)
>> +  static const Type* makeCmpResultType(const Type* opnd_type) {
>
> Instead of 'makeCmp...' please call this 'getVCmpResultType' or
> something like that.  We use 'get' for uniqued objects like types.
> Assuming that you're going to eliminate vicmp/vfcmp, this should be
> removed and you should just change its methods.

I'm a little confused here. For "make" vs "get", I agree that
"get" might be a little more consistent. As for the insertion
of the V, it's not just for vector types. And as for
removing it and changing all the methods, it's used in
four places, which seems enough to justify having this code
refactored out into a helper function.

>
> Okay, I can't review the rest of this patch without getting more
> closure on the direction we're going with the IR.  I think that
> smooshing this into icmp/fcmp is a real mistake.  We should change the
> old v[if]cmp instruction to return a vector of bool and leave icmp/
> fcmp alone.  What do you guys think?
>
> If you agree, please revert these patches for now.

I disagree. In the short term, we can't simply change v*cmp
because that would break existing users, and overloading
{i,f}cmp is relatively low-risk. In the long term we don't
want comparisons to be inconsistent with the rest of the IR.

Dan