The patch adds SPIRVLegalizerInfo, SPIRVInstructionSelector and SPIRV-specific utilities.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp | ||
---|---|---|
84–85 | This should probably be an assert, not report_fatal_error | |
215–216 | should be assert? | |
277 | No const reference | |
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h | ||
10–13 | Why do you need to do this? What are you losing in gMIR that matters? | |
llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp | ||
205–208 | You shouldn't have to construct a fresh new MIRBuilder for every selected instruction | |
519 | I agree there are too many autos | |
593–610 | I think you are just repeating the generic expansion for this. It would be better to just use the default lower action for G_ATOMIC_CMPXCHG_WITH_SUCCESS | |
596 | LLT is shorter and clearer than auto | |
661 | return after else | |
836–844 | It looks like you're repeating work done in the legalizer? | |
1102 | No else after return | |
1139 | Should just use cast<> and remove the check, this should never fail. I believe we disallow null G_GLOBAL_VALUE sources | |
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp | ||
178 | I thought SPIRV had a fixed set of register types? | |
203–204 | This is illegal MIR, there's no point in checking it | |
205 | Else after return | |
206 | Typo ponter | |
218–219 | You don't need to worry about the cases that are illegal MIR | |
220 | Else after return | |
234 | This has been not true for about 2 years | |
287 | I'm not sure what's meant by an "Id Reg" | |
302 | Are you intentionally discarding the pointer size and address space? | |
311 | Generally code in the legalizer shouldn't bother setting regbanks or classes | |
333 | You're doing a lot of work to convert to the SPIRV types here, which would be more appropriate to do during selection | |
349–356 | it might be cleaner to push this cast to integer into the generic LegalizerHelper | |
362–366 | You're assuming there's only a single use, and your legalize action should only touch the passed instruction | |
llvm/lib/Target/SPIRV/SPIRVUtils.cpp | ||
133–134 | How/why are you not using generic as addrspace 0? | |
161–170 | I dislike this wrapper function. You should have all of these variables available in the context where you are constraining already, and the MF parameter with null check is potentially confusing |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h | ||
---|---|---|
777 | Ok, "assert((Op.getImm() >> 32) == 0..." will be added. | |
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h | ||
10–13 | SPIR-V annotates each instruction with type IDs. SPIR-V uses high-level types including composites (structs, arrays...) and some others. Most of them can be obtained from LLVM Types but they are not available in gMIR, so we need to save and maintain own type information. | |
llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp | ||
519 | The autos will be cleaned. | |
836–844 | Yes, it looks like I need to fix this. | |
1139 | Ok, I'll check this. | |
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp | ||
30 | It will be replaced by std::set. | |
llvm/lib/Target/SPIRV/SPIRVUtils.cpp | ||
133–134 | One of the important features of this backend is compatibility with SPIRV-LLVM-Translator. We follow the address spaces agreement announced in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/docs/SPIRVRepresentationInLLVM.rst#address-spaces . |
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp | ||
---|---|---|
178 | In SPIRV [almost] every value is an ID which is just an integer (usually 32- or 64-bit ), the type of the value with an ID is detemined by the instruction producing it which has 'ResultType' parameter. But we're trying not to enforce that rule on gMIR level so that before the selection all of the types were the ones IRTranslator emitted without anyhting target-specific interfering. | |
302 | Well, yes and no. In fact LLTs do not matter after we generated all the necessary type-assigning pseudo instructions. But LLTs still matter in sence of gMIR being well-formed (passing verification) and not having any significant differences from the original IR (i.e. scalars instead of vectors, vectors of different type, etc) The addrspace of LLT doesn't matter for sure | |
333 | We placed this code here because it's doing some sort of legalization (in old SPIR-V versions pointers can't be compared directly, ptrtoints are required), not sure if it's good to mix things up in the selector. | |
362–366 | The single-use constraint is enforced by the design of our target, every non-void instruction have the only user - type assignment instruction, which in turn may have multiple users just as the original instruction before. |
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp | ||
---|---|---|
178 | That's the whole point of the legalizer. There's some notion of fixed supported types implied by the other legalizer rules | |
362–366 | This is not a property the target can decide, the incoming IR can have multiple uses. It's simply wrong to rely on it like this |
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp | ||
---|---|---|
362–366 | Why not if we have a target-specific LLVM IR pass making that assumption always true? |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp | ||
---|---|---|
29 | Ok, it will be applied to other expanded macros that also return literals. | |
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp | ||
135–145 | I'm not sure that it is checked by verifier. | |
175 | Thanks, it will be fixed. | |
llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp | ||
205–208 | As I understand you suggest constructing MachineIRBuilder closer to its users, because not all instructions (for selection) are actually use it. | |
593–610 | Yes, I'll fix it. | |
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp | ||
178 | @arsenm yes, all register types are legal for phis. This rule will be substituted by legalFor(allPtrsScalarsAndVectors). | |
287 | I suppose it means a register which is used in GET_ID pseudo instruction. | |
302 | It's used in creation of new temporary pseudos which are involved in the instruction selection and then are removed. Perhaps it's a bad practice and should be avoided. | |
311 | Probably it would be better to move this to selector. | |
333 | ||
349–356 | Ok, I have created a function that generates a single cast. | |
362–366 | Instructions processed here should always have a single use which is a pseudo instruction ASSIGN_TYPE, generated by SPIRV specific pass before the legalizer. We'll add a check that this condition is met. Also it looks like the modification of another instructions may be avoided. | |
llvm/lib/Target/SPIRV/SPIRVUtils.cpp | ||
161–170 | Ok, we'll get rid of it. |
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp | ||
---|---|---|
362–366 | Oh, I see. It's just the pass responsible for making the input IR compliant with what SPIR-V specific GlobalISel stages expect is currently missing from these patches. Maybe we need to extend the series |
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp | ||
---|---|---|
362–366 | As Renato suggested in the comment (see the first patch), we need a minimum set of patches that does something substantial. Then we'll add passes/features by stand alone patches extending the SPIR-V backend functionality and making more tests passed. I'm not sure that right now we have a reason to increase the series noticeably. |
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp | ||
---|---|---|
362–366 | Yep, I understand that. The question is whether initial series should be kind of transitive closure or there might be some logical inconsistency due to some parts of the code not being upstreamed yet. |
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp | ||
---|---|---|
362–366 | Of course the series should be consistent but I think it doesn't make sense for us to strive for transitive closure at this stage, because it is only the first step towards adding (to LLVM) a full-featured SPIR-V backend, which will be useful for end users. Approaching this goal in accordance with the logic of the LLVM project is the meaning of the current work. Probably it's worth adding more functionality to SPIRVGlobalTypesAndRegNumPass.cpp so more tests from our test base can initially pass. But as Matt commented we should make changes in the way SPIRVGlobalTypesAndRegNumPass works, I'm going to try it. |
The issues found by Renato Golin, Fangrui Song and Matt Arsenault were fixed. The macros in BaseInfo were expanded.
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp:532:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
The warning was fixed.
Also addNumImm function was corrected.
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp | ||
---|---|---|
1407 | No else after return | |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h | ||
23 | SPIRV namespace? | |
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp | ||
175 | the static_cast to function is also no good since this could be a constexpr cast | |
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp | ||
151 | Use cast here and remove the assert below? | |
160 | no const_casts? |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h | ||
---|---|---|
23 | Probably you mean namespace llvm { namespace SPIRV { namespace Capability { |
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp | ||
---|---|---|
160 | Module is fixed, but I'm not sure how to cast const Type *Ty to Type * without const_cast. I see that other targets uses this approach in the similar cases (ARM, Hexagon). |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h | ||
---|---|---|
26 | namespace XXX { enum XXX { ... } } can usually be replaced with enum class XXX {...}` |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp | ||
---|---|---|
283 | I think these all these switch functions need to move the unknown case to default and use llvm_unreachable after the switch to avoid warnings on all compilers | |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h | ||
521 | Don't need llvm:: | |
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp | ||
37 | getSubtarget<SPIRVSubtarget>.I think it's worthwhile to just have this as a permanent member of SPIRVCallLowering | |
164 | This should still return false since multiple registers still can come from valid IR | |
172 | This will still fail on constexpr casts and indirect calls. dyn_cast_or_null and return if you don't want to handle now | |
176 | You should not construct one off MachineIRBuilders. Re-use the one passed in and adjust the insert point. There's additional state (like CSEInfo) you're losing | |
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp | ||
339 | Why all the null checks on Type? I would expect all of these to require non-null inputs | |
llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp | ||
205–208 | No, I mean MachineIRBuilder is a heavy class that should have state set up and maintained. It's not something to construct for a single use. The other selectors mostly just use BuildMI directly during selection. |
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp | ||
---|---|---|
37 | Well, it looks reasonable. | |
172 | Thanks, it'll be fixed. | |
176 | Sure, this is bad code. | |
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp | ||
339 | Thanks, will fix. | |
llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp | ||
205–208 | Thanks, now I see that only AMDGPU and Mips targets sometimes use MachineIRBuilder. I'll change it in SPIRV. |
llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp | ||
---|---|---|
205–208 | I have left only one (relatively rare) case of using MachineIRBuilder in SPIRVInstructionSelector. I suppose this is acceptable since AMDGPU and Mips do it in a similar way. |
LGTM with nits
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp | ||
---|---|---|
284 | the llvm_unreachable is a bit pointless without a break here | |
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp | ||
93–95 | getParamAlignment would be nicer | |
llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp | ||
205–208 | I've been meaning to fix those; I wouldn't use that as a good example. | |
811 | Braces | |
996 | Braces. Line wrapping is a bit weird looking | |
1041 | Not much point asserting on things the verifier checks |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp | ||
---|---|---|
284 | I see, will change to break. | |
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp | ||
93–95 | It'll be corrected. | |
llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp | ||
205–208 | Ok, if you don't mind I'll add FIXME for this last case for now and fix it as soon as possible. This fix may require an additional extension in SPIRVGlobalRegistry, in which case I would commit it after this initial series of patches. | |
1041 | Will remove. |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp | ||
---|---|---|
381 | Initially all these functions were generated using macros. Then Renato suggested getting rid of them and we agreed to use expanded macros for now. Later we'll re-implement all the functions with tablegen (this work is in progress so I expect it to be completed in several months). I have already fixed a number of issues in these functions after expanding the macros. I'm not sure that now we really need to replace them with new macros. |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp | ||
---|---|---|
381 | Ah, ok. @rengolin I feel that X macros may be more readable and less error-prone than open coding here. TableGen can generate such strings as well, but it will add one step, bring a little abstraction code, and is probably not conciser than macros here (AIUI one line for one enumeration value). WDYT? |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp | ||
---|---|---|
381 | The recent clang-format discussion is also relevant here. I think the current open coding can be perceived as error-prone by some folks: |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp | ||
---|---|---|
381 |
As I know we'll have all these functions fully generated by TableGen, so it will be quite concise. In the first version we had quite complex macros for all BaseInfo entities, and even more complex macros in the original Khronos repository, so we shouldn't return to that implementation. I assume that you are now suggesting using very simple macros like this: #define CASE(CLASS, MEMBER) \ case CLASS::MEMBER: return #MEMBER; ... StringRef SPIRV::getExecutionModeName(ExecutionMode e) { switch (e) { CASE(ExecutionMode, Invocations) I'm not sure whether we need this at the moment, so I set TODO before all the functions for now. However if you think it's a better solution then we have now, no problem, I'll change all the functions (with this open coding). But we all need to agree on this. |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp | ||
---|---|---|
20 | why not use: namespace llvm { namespace SPIRV { and then avoid prefixing all types with SPIRV? | |
381 | I honestly can't remember what was there before, but this huge list of strings does seem a bit out of hand. With macros, we'd still have a huge list of calls, but we wouldn't have the opportunity to make typo mistakes (if using X(str) case str: return "#str";). I'm also not sure why you need all of these strings at this level, unless you want to print all that in your assembly output. Is that the point? Or is it just debug? |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp | ||
---|---|---|
20 | You are right, we can avoid SPIRV prefix. | |
381 |
Ok, I'll change this to X macros.
Yes, we need to print all these operands in the assembly output. Anyway the TableGen version is in progress, so this huge list will be removed. |
Thanks @iliya-diyachkov, it looks cleaner, even if still very long. A table generated output will hopefully fix this soon.
First, sorry for the confusion if I made you do something only to revert later, but that shows the value of multi-person reviews. So, also thanks others for picking up on that.
I have no further comments, this looks good to me now. Thanks!
Thanks, Renato!
First, sorry for the confusion if I made you do something only to revert later, but that shows the value of multi-person reviews. So, also thanks others for picking up on that.
No problem. The initial version of macros was more complicated, so it was not just a return, but an adaptation of the original code.
Now we have to finish the review of the 6th patch. All the issues were fixed about a week ago, I hope Matt will respond soon.
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp | ||
---|---|---|
286 | This is incompatible with opaque pointers (https://llvm.org/docs/OpaquePointers.html). Please remove this call or revert the patch if this is non-trivial. |
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp | ||
---|---|---|
286 | What if we use PointerType::getNonOpaquePointerElementType() instead? IMO that should do the trick while SPIR-V has no support of opaque pointers yet so we're only oging non-opaque way here. |
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp | ||
---|---|---|
286 | You can use getNonOpaquePointerElementType(), but you need to implement a separate code-path for isOpaquePointerTy() if you do so. |
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp | ||
---|---|---|
286 | Sure, we'll implement. Thanks, Nikita. |
SPIRV namespace?