Re: [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  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  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 pMem = 
>> MemoryBuffer::getMemBuffer(StringRef((const char*)pIR), "");
>> -
>> -SMDiagnostic Err;
>> -std::unique_ptr 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 
>> return CALLA(Callee, args);
>> }
>> 
>> +#if HAVE_LLVM > 0x306
>> +CallInst *Builder::CALL(Value *Callee, Value* arg)
>> +{
>> +   std::vector args;
>> +   args.push_back(arg);
>> +   return CALLA(Callee, args);
>> +}
>> +
>> +CallInst *Builder::CALL2(Value *Callee, Value* arg1, Value* arg2)
>> +{
>> +   std::vector 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 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 

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

[Emil Velikov]

Hi Tim,

Does this commit allows us to have generic generated files or it's
solely workaround the VPERMD intrinsic argument swap ?

On 17 June 2016 at 20:25, Tim Rowley  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.


> @@ -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 pMem = 
> MemoryBuffer::getMemBuffer(StringRef((const char*)pIR), "");
> -
> -SMDiagnostic Err;
> -std::unique_ptr 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.


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


>  // 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.


>  #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.


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


> @@ -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 ?


> @@ -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
> 

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

[Tim Rowley]

---
 src/gallium/drivers/swr/Makefile.am|  2 +
 .../drivers/swr/rasterizer/jitter/JitManager.cpp   | 33 +---
 .../drivers/swr/rasterizer/jitter/JitManager.h | 22 ---
 .../drivers/swr/rasterizer/jitter/blend_jit.cpp| 13 ++-
 .../drivers/swr/rasterizer/jitter/builder_misc.cpp | 44 ++
 .../drivers/swr/rasterizer/jitter/builder_misc.h   |  8 +++-
 .../drivers/swr/rasterizer/jitter/fetch_jit.cpp| 18 ++---
 .../jitter/scripts/gen_llvm_ir_macros.py   | 11 +-
 .../swr/rasterizer/jitter/streamout_jit.cpp|  9 +
 9 files changed, 84 insertions(+), 76 deletions(-)

diff --git a/src/gallium/drivers/swr/Makefile.am 
b/src/gallium/drivers/swr/Makefile.am
index d896154..92e03fb 100644
--- a/src/gallium/drivers/swr/Makefile.am
+++ b/src/gallium/drivers/swr/Makefile.am
@@ -98,6 +98,7 @@ rasterizer/jitter/builder_x86.h: 
rasterizer/jitter/scripts/gen_llvm_ir_macros.py
$(MKDIR_GEN)
$(PYTHON_GEN) \
$(srcdir)/rasterizer/jitter/scripts/gen_llvm_ir_macros.py \
+   --llvm-version $(LLVM_VERSION) \
--output rasterizer/jitter/builder_x86.h \
--gen_x86_h
 
@@ -105,6 +106,7 @@ rasterizer/jitter/builder_x86.cpp: 
rasterizer/jitter/scripts/gen_llvm_ir_macros.
$(MKDIR_GEN)
$(PYTHON_GEN) \
$(srcdir)/rasterizer/jitter/scripts/gen_llvm_ir_macros.py \
+   --llvm-version $(LLVM_VERSION) \
--output rasterizer/jitter/builder_x86.cpp \
--gen_x86_cpp
 
diff --git a/src/gallium/drivers/swr/rasterizer/jitter/JitManager.cpp 
b/src/gallium/drivers/swr/rasterizer/jitter/JitManager.cpp
index 4bbd9ad..03b616e 100644
--- a/src/gallium/drivers/swr/rasterizer/jitter/JitManager.cpp
+++ b/src/gallium/drivers/swr/rasterizer/jitter/JitManager.cpp
@@ -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"
 
 #if defined(_WIN32)
@@ -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 pMem = 
MemoryBuffer::getMemBuffer(StringRef((const char*)pIR), "");
-
-SMDiagnostic Err;
-std::unique_ptr 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;
-}
-
 
 //
 /// @brief Dump function x86 assembly to file.
@@ -281,7 +252,7 @@ void JitManager::DumpAsm(Function* pFunction, const char* 
fileName)
 sprintf(fName, "%s.%s.asm", funcName, fileName);
 #endif
 
-#if HAVE_LLVM == 0x306
+#if HAVE_LLVM <= 0x306
 raw_fd_ostream fd(fName, EC, llvm::sys::fs::F_None);
 formatted_raw_ostream filestream(fd);
 #else
diff --git a/src/gallium/drivers/swr/rasterizer/jitter/JitManager.h 
b/src/gallium/drivers/swr/rasterizer/jitter/JitManager.h
index 14ba893..aaedf89 100644
--- a/src/gallium/drivers/swr/rasterizer/jitter/JitManager.h
+++ b/src/gallium/drivers/swr/rasterizer/jitter/JitManager.h
@@ -29,16 +29,16 @@
 **/
 #pragma once
 
-#include "common/os.h"
-#include "common/isa.hpp"
-
 #if defined(_WIN32)
 #pragma warning(disable : 4146 4244 4267 4800 4996)
 #endif
 
 // llvm 3.7+ reuses "DEBUG" as an enum value
+#if defined(DEBUG)
 #pragma push_macro("DEBUG")
 #undef DEBUG
+#define _DEBUG_COLLISSION
+#endif // DEBUG
 
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Instructions.h"
@@ -54,7 +54,7 @@
 #endif
 
 #ifndef HAVE_LLVM
-#define HAVE_LLVM (LLVM_VERSION_MAJOR << 8) || LLVM_VERSION_MINOR
+#define HAVE_LLVM ((LLVM_VERSION_MAJOR << 8) | LLVM_VERSION_MINOR)
 #endif
 
 #include "llvm/IR/Verifier.h"
@@ -64,10 +64,14 @@
 
 #include "llvm/Analysis/Passes.h"
 
-#if HAVE_LLVM == 0x306
+#if HAVE_LLVM <= 0x306
 #include "llvm/PassManager.h"
+using FunctionPassManager = llvm::FunctionPassManager;
+using PassManager = llvm::PassManager;
 #else
 #include "llvm/IR/LegacyPassManager.h"
+using FunctionPassManager = llvm::legacy::FunctionPassManager;
+using PassManager = llvm::legacy::PassManager;
 #endif
 
 #include "llvm/CodeGen/Passes.h"
@@ -79,7 +83,14 @@
 #include "llvm/Support/Host.h"
 
 
+#if