VectorBuilder wraps around an IRBuilder and VectorBuilder::createVectorInstructions emits VP intrinsics as if they were regular instructions.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
llvm/lib/IR/VectorBuilder.cpp | ||
---|---|---|
31 | Doesn't BasicBlock have getModule? |
llvm/include/llvm/IR/VectorBuilder.h | ||
---|---|---|
93 | Aren't Twine's usually passed by const reference? |
llvm/include/llvm/IR/VectorBuilder.h | ||
---|---|---|
27 | Do we really need this typedef? Mainly asking because it went into the llvm namespace. | |
29 | Can we move this into the class so it becomes VectorBuilder::Behavior? | |
84 | How do you set StaticVectorLength to a scalable value? | |
llvm/lib/IR/VectorBuilder.cpp | ||
76 | Does this assume the Mask and EVL are at the end? If they aren't then ParamIdx isn't the correct index here if the Mask and EVL were earlier. Assuming there aren't empty slots for them in InstOpArray. If they are always at the end and we're going to assume, that then there's not a lot of point for handling them inside the loop. You can just copy the InstOps, then add mask and evl if they exist. | |
llvm/unittests/IR/VectorBuilderTest.cpp | ||
22 | nit: int->unsigned. That's at least what most of the interfaces this gets passed to use. | |
32 | Use std::make_unique instead of creating a unique_ptr for the return. |
vp.merge and vp.select do not ignore masked-off lanes. We probably shouldn't treat their mask parameters as "masks" in the VP sense. I suggest we stop reporting their MaskPos in VPintrinsics.def.
llvm/include/llvm/IR/VectorBuilder.h | ||
---|---|---|
84 | Scalable types aren't supported in this first patch. Planned for a followup. | |
llvm/lib/IR/VectorBuilder.cpp | ||
76 | There is an issue with ParamIdx at the moment. If the loop is past the first mask or evl parameter the ParamIdx will be off by one for the other one. It is okay for the other parameters though (note that there is no else in front of the last if). |
llvm/lib/IR/VectorBuilder.cpp | ||
---|---|---|
87 | This loop doesn't work if VL is before mask and they are adjacent to each other. VL will increment VPParamIdx, but don't re-check mask after adding VL. I think the for statement should be reponsible for incrementing VPParamIdx only. And ParamIdx should only be incremented in the loop body then a parameter from InstOpsArray is pushed. There should be no increments of VPParamIdx in the loop body. |
Improved parameter list construction following @craig.topper 's suggestion. Also, fast path (both mask and evl at tail) improved to allow for any ordering of the two.
LGTM you can address the comments if you want, but I don't want to hold this up anymore.
llvm/lib/IR/VectorBuilder.cpp | ||
---|---|---|
81 | What if we just called IntrinParams.resize(NumVPParams); and then assigned the mask and vl into their slots? | |
llvm/unittests/IR/VectorBuilderTest.cpp | ||
131 | Can simplify a little by merging the isa and cast to a single dyn_cast. static bool isLegalConstEVL(Value *Val, unsigned ExpectedEVL) { auto *ConstEVL = dyn_cast<ConstantInt>(Val); if (!ConstEVL) return false; // Value check. if (ConstEVL->getZExtValue() != ExpectedEVL) return false; // Type check. return ConstEVL->getType()->isIntegerTy(32); } Or go really packed and do. static bool isLegalConstEVL(Value *Val, unsigned ExpectedEVL) { auto *ConstEVL = dyn_cast<ConstantInt>(Val); return ConstEVL && ConstEVL->getZExtValue() == ExpectedEVL && ConstEVL->getType()->isIntegerTy(32); } |
Breaking the clang-cmake-x86_64-avx2-linux bot: https://lab.llvm.org/buildbot/#/builders/110/builds/10912
I suspect Behavior Behavior = Behavior::[..] doesn't go down well with that version of gcc. Waiting to find out which version of gcc is used there to reproduce.
Just a note, this also breaks implicit module build as the newly added header indirectly includes llvm/IR/Attributes.inc which needs to be generated first. If you do a module build (-DLLVM_ENABLE_MODULES=ON), you are likely to get error:
09:05:38 While building module 'LLVM_MC' imported from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/lib/MC/MCAsmBackend.cpp:9: 09:05:38 While building module 'LLVM_IR' imported from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/MC/MCPseudoProbe.h:49: 09:05:38 While building module 'LLVM_intrinsic_gen' imported from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/IR/VectorBuilder.h:18: 09:05:38 In file included from <module-includes>:1: 09:05:38 In file included from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/IR/Argument.h:17: 09:05:38 /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/IR/Attributes.h:75:14: fatal error: 'llvm/IR/Attributes.inc' file not found 09:05:38 #include "llvm/IR/Attributes.inc" 09:05:38 ^~~~~~~~~~~~~~~~~~~~~~~~ 09:05:38 While building module 'LLVM_MC' imported from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/lib/MC/MCAsmBackend.cpp:9: 09:05:38 While building module 'LLVM_IR' imported from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/MC/MCPseudoProbe.h:49: 09:05:38 In file included from <module-includes>:62: 09:05:38 /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/IR/VectorBuilder.h:18:10: fatal error: could not build module 'LLVM_intrinsic_gen' 09:05:38 #include <llvm/IR/IRBuilder.h> 09:05:38 ~~~~~~~~^
You probably need to do something in the module map to fix this (see: module LLVM_intrinsic_gen in llvm/include/llvm/module.modulemap )
llvm/include/llvm/IR/VectorBuilder.h | ||
---|---|---|
22 | Is anything from PatternMatch being used? |
llvm/include/llvm/IR/VectorBuilder.h | ||
---|---|---|
22 | Definitely not in this header, no |
- Add module map entry
- Rename Behavior variable (this may be confusing gcc 5.4; not confirmed)
- Remove PatternMatch include
Is anything from PatternMatch being used?