[Mesa-dev] [PATCH 05/14] swr: [rasterizer jitter] cleanup supporting different llvm versions

[Rowley, Timothy O]

Looks like some major rework of this commit is in order; I’ll get on it.

> On Jun 17, 2016, at 3:36 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 
> Hi Tim,
> 
> Does this commit allows us to have generic generated files or it's
> solely workaround the VPERMD intrinsic argument swap ?

Since the arguments are both llvm::Value*, either version of the generated files would work.  See further discussion later on...

> 
> On 17 June 2016 at 20:25, Tim Rowley <timothy.o.rowley at intel.com> wrote:
> 
>> @@ -31,8 +31,8 @@
>> #pragma warning(disable: 4800 4146 4244 4267 4355 4996)
>> #endif
>> 
>> -#include "jit_api.h"
>> #include "JitManager.h"
>> +#include "jit_api.h"
>> #include "fetch_jit.h"
>> 
> Is this due to the DEBUG redefinition ? One might want to add a
> comment, to prevent the next person from moving/breaking things.

Someone’s independent discovery of the DEBUG issue, I think.

> 
>> @@ -222,35 +222,6 @@ void JitManager::SetupNewModule()
>>     mIsModuleFinalized = false;
>> }
>> 
>> -//////////////////////////////////////////////////////////////////////////
>> -/// @brief Create new LLVM module from IR.
>> -bool JitManager::SetupModuleFromIR(const uint8_t *pIR)
>> -{
>> -    std::unique_ptr<MemoryBuffer> pMem = MemoryBuffer::getMemBuffer(StringRef((const char*)pIR), "");
>> -
>> -    SMDiagnostic Err;
>> -    std::unique_ptr<Module> newModule = parseIR(pMem.get()->getMemBufferRef(), Err, mContext);
>> -
>> -    if (newModule == nullptr)
>> -    {
>> -        SWR_ASSERT(0, "Parse failed! Check Err for details.");
>> -        return false;
>> -    }
>> -
>> -    mpCurrentModule = newModule.get();
>> -#if defined(_WIN32)
>> -    // Needed for MCJIT on windows
>> -    Triple hostTriple(sys::getProcessTriple());
>> -    hostTriple.setObjectFormat(Triple::ELF);
>> -    newModule->setTargetTriple(hostTriple.getTriple());
>> -#endif // _WIN32
>> -
>> -    mpExec->addModule(std::move(newModule));
>> -    mIsModuleFinalized = false;
>> -
>> -    return true;
>> -}
>> -
>> 
> This (and the respective prototype) seems to be completely unrelated cleanup.

This method had a change which wasn’t building, but as it was unused code it seemed simpler to just drop it.

>> -#if HAVE_LLVM == 0x306
>> +#if HAVE_LLVM <= 0x306
> SWR requires LLVM 3.6 so all of these changes are not needed.

Fair point.

>> // llvm 3.7+ reuses "DEBUG" as an enum value
>> +#if defined(DEBUG)
>> #pragma push_macro("DEBUG")
>> #undef DEBUG
>> +#define _DEBUG_COLLISSION
>> +#endif // DEBUG
>> 
> I believe Jose mentioned a slightly better way to handle this (see
> src/gallium/auxiliary/gallivm/lp_bld_misc.cpp). Or that does not apply
> here ? Also it would be nice to keep it a separate patch as you did
> earlier.

Will clean up.

>> #ifndef HAVE_LLVM
>> -#define HAVE_LLVM (LLVM_VERSION_MAJOR << 8) || LLVM_VERSION_MINOR
>> +#define HAVE_LLVM ((LLVM_VERSION_MAJOR << 8) | LLVM_VERSION_MINOR)
>> #endif
>> 
> Unrelated bugfix, stable material.

Doesn’t affect Mesa as the build system defines HAVE_LLVM.  But definitely a “whoops” to fix.

>> @@ -322,6 +322,32 @@ CallInst *Builder::CALL(Value *Callee, const std::initializer_list<Value*> &args
>>     return CALLA(Callee, args);
>> }
>> 
>> +#if HAVE_LLVM > 0x306
>> +CallInst *Builder::CALL(Value *Callee, Value* arg)
>> +{
>> +       std::vector<Value*> args;
>> +       args.push_back(arg);
>> +       return CALLA(Callee, args);
>> +}
>> +
>> +CallInst *Builder::CALL2(Value *Callee, Value* arg1, Value* arg2)
>> +{
>> +       std::vector<Value*> args;
>> +       args.push_back(arg1);
>> +       args.push_back(arg2);
>> +       return CALLA(Callee, args);
>> +}
>> +
>> +CallInst *Builder::CALL3(Value *Callee, Value* arg1, Value* arg2, Value* arg3)
>> +{
>> +       std::vector<Value*> args;
>> +       args.push_back(arg1);
>> +       args.push_back(arg2);
>> +       args.push_back(arg3);
>> +       return CALLA(Callee, args);
>> +}
>> +#endif
>> +
> These seem unused ?

Here I have to give the answer you hate: unused by the public rasterizer, used by some internal code.  These seemed small enough generic helper methods that might have future use by us as well that I thought it simpler to leave them in.

>> @@ -726,8 +752,12 @@ Value *Builder::PERMD(Value* a, Value* idx)
>>     // use avx2 permute instruction if available
>>     if(JM()->mArch.AVX2())
>>     {
>> -        // llvm 3.6.0 swapped the order of the args to vpermd
>> -        res = VPERMD(idx, a);
>> +#if (HAVE_LLVM == 0x306) && (LLVM_VERSION_PATCH == 0)
>> +               // llvm 3.6.0 swapped the order of the args to vpermd
>> +               res = VPERMD(idx, a);
>> +#else
>> +               res = VPERMD(a, idx);
>> +#endif
> Something feels rather fishy here -  atm generator was always creates
> one 'version' of the intrinsic and the code directly uses is.
> 
> With this patch (taking that I understood the python script correctly):
> - Here we swap the order depending of the LLVM used to compile SWR
> - In the generator (below) we also swap the order.
> 
> If the same version of LLVM is used to generate and compile. if not
> things just go boom. Considering that this is the gist of the patch,
> wouldn't it be nice to have a bit more comprehensive comment be that
> here, below and/or in the commit message ?

Took me a while of looking at this change (I’m not the original author) to come up with the intent.  The python generator change is cosmetic to name the “idx” and “a” variables so that if you look at the header it’s clear what it does.  The code change in builder_misc.cpp flips the args to match.

Perhaps a cleaner way would be to remove the builder_misc #if, change the signature to VPERMD to the correct version, and have the generator cpp chunk be responsible for flipping the args?  I could make that check so that builder_gen.cpp/h doesn’t need to created specific to a llvm version.

> 
> 
>> @@ -374,8 +374,17 @@ def main():
>>     parser.add_argument("--gen_cpp", "-gen_cpp", help="Generate builder_gen.cpp", action="store_true", default=False)
>>     parser.add_argument("--gen_x86_h", "-gen_x86_h", help="Generate x86 intrinsics. No input is needed.", action="store_true", default=False)
>>     parser.add_argument("--gen_x86_cpp", "-gen_x86_cpp", help="Generate x86 intrinsics. No input is needed.", action="store_true", default=False)
>> +    parser.add_argument("--llvm-version", help="What LLVM version to generate for", action="store", default="3.6.0")
>> +
>>     args = parser.parse_args()
>> 
>> +    if args.llvm_version == "3.6.0":
>> +        # Swap args for VPERMD
>> +        for i in range(len(intrinsics)):
>> +            if intrinsics[i][0] == "VPERMD":
>> +                intrinsics[i][2] = [intrinsics[i][2][1], intrinsics[i][2][0]]
>> +                break
>> +
> If one needs this hunk, may I suggest that one drops the default value
> and just errors out if the caller of the script hasn't provided a
> llvm-version ?
> 
> Thanks
> Emil