-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Should enhance LLVM switch instruction to take case ranges #1627
Comments
That's definitely that thing, I've thought about after discussing with Duncan In fact, it will make switch instruction much more "natural" and will eliminate |
Maybe this can be added to the project page as something suitable for |
Maybe, however Open Projects page already mention "code-cleanup" bugs. Maybe |
I'm currently thinking about implementing this feature (otherwise we can simple Main question is: how should case ranges implemented. It seems, that these
Currently I'm planning to play with some specialized version of ConstantStruct. |
I don't think that CaseRange should be a value. I think that Switch should just have triples of operands. -Chris |
I don't understand your objection to ConstantRange. First off let me Secondly, there's another reason to handle wrapping ranges. The root cause If the LLVM switch stuff is going to have to handle wrapped ranges anyway, I |
Ah, I don't object to constant range. To be more clear, I object to making ConstantRange itself be a Actually the problem I think is that we currently represent the value as an explicit operand of switch. Does that make sense? -Chris |
FWIW case ranges are a language feature in standard Fortran. Does this mean that this bug needs to be resolved before llvm-gfortran can work, or does LLVM currently lower switch ranges produced by the gcc front ends to an enumeration of all case values? |
Yes. Exactly. With some threshold value: if case range is too big it emits just range comparison and 'if'. |
The SWITCH_EXPR lowering code in llvm-gcc currently converts case ranges to an enumerated set of cases in an llvm switch instruction if the case range is less than 64 elements. If it is more than 64 elements, it emits an "if" before the switch to evaluate the case range. This is not necessarily optimal, but is fully correct. |
Hi, guys. What the state of case ranges is by now? |
No one did anything. |
May be we can use ConstantArray instead of ConstantRanges? Something like this: switch i32 %val, label %otherwise [ i32 0, label %onzero We also can simplify syntax in this case omitting [2 x i32]: switch i32 %val, label %otherwise [ i32 0, label %onzero |
So what about moving case-values to another collection? I can try to do this if no-one against... |
FWIW, I don't think that using ConstantArray's here make sense. Instead, SwitchInst should move to not using Value*'s to represent the arguments, they should be a non-Value* datastructure that just contains APInts or something. |
It seems that some optimizations becames impossible after case values was moved to non-operands collection. I mean optimizations that replaces Use objects with more optimal ones. "CodeGenPrepare::OptimizeExtUses" for example do it (CodeGenPrepare.cpp, string 1054). |
I don't understand what you are saying. Currently switch cases are ConstantInt, |
Switch condition is stored like a case value too, with idx=0. It is may be a non-constant value. So, it is may be optimized. |
I don't think anyone was suggesting changing the way the switch condition is |
OK. What about next format of op-list: |
|
Right. Also, note that changing the storage of the case values in memory doesn't mean that the syntax in .ll files has to change. |
I have two concerns about the current design: First, the implementation of SwitchInst::CaseIteratorT::getCaseValue() seems to be buggy, since it only iterates over the Low value of each range. It should keep state and be able to iterate over all elements of each case range. Second, I couldn't find a simple way to cast the output of SwitchInst::CaseIteratorT::getCaseValueEx() (a ConstantRangesSet) to a ConstantRange, which seems to be the most natural object for users that can actually handle case range values. Can this be made available? |
SwitchInst::CaseIteratorT::getCaseValue() is deprecated old style method. It is kept for unchanged passes that are not adopted to new case ranges feature. About ConstantRangesSet and ConstantRange. It is a slightly different classes. Don't try to convert CRS to CR. // Example of enumeration ranges in case // Example of looking up for case that satisfies condition: Also note that after all passes will updated and SwitchInst will work with real cases, the ConstantRangesSet will rebased from ConstantInt numbers to the APInt numbers. |
Of course the last example is a little bit redundant since you can use |
I guess a missed the discussion on the mailing list. |
I think the name ConstantRangeSet is unfortunate. In fact I think it should I think the implementation is also unfortunate, i.e. poor. Why is it mucking template struct CRSConstantTypes { template <> WTF? And this is only on the first screen, I didn't have the courage to look |
Alternatively, if ConstantRangesSet is not a real abstract data type, but just a template <class SwitchInstTy, class ConstantIntTy, class BasicBlockTy>
public:
and so on. I would love it if that could all be replaced with a forward |
Duncan, you right million times. Wrapping concept is temporary and looks terrible. So all this stuff like Relative to Instruction.h - OK. I'll make it with forward declared. The strategy of case ranges implementation:
|
OK, thanks for explaining! |
I've reverted Stepan's patches in llvm svn 190328. It would still be great to add this feature sometime, though! |
I'd like to work on implementing this, if it is still a desried feature, and if no one else is currently working on this. Might need some help along the way though :) |
Don't repeat my mistakes! Don't ever try to enhance existing SwitchInst. For this way supposes simultaneous enhancement of existing passes. A lot of them. You can't just add new feature to SwitchInst and keep the rest of llvm unaffected. I would rather suggest you to introduce new instruction. And at first just to lower it to set of condition blocks. Then you could turn on its support in passes one by one. Here is my 6 years old code with sketches for new 'switchr' instruction, which could be helpfull to you: https://github.com/kaomoneus/llvm.stepan.dyatkovskiy/tree/switchr |
Hi Stepan, Thanks a lot for letting me know, and also for sharing the old code, I know it'll be really helpful. The idea of introducing a new instruction seems good to me too, should I start working on it? Or should I wait for other people's review on this idea? |
Also, does the old code contains your work on changing the switch instruction too? Or does it contain only the switchr sketches? Missed the reply button last time ':) |
This code contains only new instruction and related modifications. I haven't checked it for years, but I believe it's compilable and runnable. I tried not to touch rest of llvm on purpose. Any line of new code in core should be done very carefully, for you have a lot of different The best way to check what was done there is to compare 'switchr' branch with those-days master branch. Smth like this: As for waiting for review. This is definitely should be reviewed and approved. But may be you could start prototyping already, so then you would had something at your hands during review discussion. |
*for you have a lot of different buildbots with different settings and targets. And each of them should be green (successful) all the time. Also Think about some special trigger which allows to disable maximum of your code for legacy behaviour. |
Thanks a lot for your help, I will be checking out the diff between the master and the switchr, and will ask you if I am confused about something. |
Sure! You're welcome! |
Hi Stepan, Also the first question: /// If we have flat and ordered collection of numbers, then range I couldn't really understand what does collection actually represent. |
If collections represents the collection of numbers used with the switchr instruction, what does it being wrapped mean? |
Honestly I should check code. But something I remember. "Flat", well, I think that just means single-dimensional. So it's not flat, it's rather.. single dimensional :-) Basically I meant just an ordered sequence of numbers. Wrapped range Low=collection[collection.size()-1], High=collection[0] perhaps means this: [ [collection.size()-1], +inf ) U (-inf, collection[0] ) So it's universum with the rest of ranges excluded. And the last statement better to check in code. I can do it only at monday though. |
I don't speak for Stepan, but this advice isn't obviously correct to me. I understand and agree with his point that "updating all the passes is a lot of work" but it isn't clear to me that the LLVM community with accept a new "switch with ranges" instruction. It seems better to define a new global "eliminate switch ranges" operation that takes a SwitchInst, and update all passes to either a) work with ranges, or b) call this helper. Then file a bug that says "phase out all calls to the 'eliminate switch ranges' helper". |
By an "eliminate switch ranges" operation, do you mean rewriting the IR, replacing a switch with ranges with (the equivalent of) a bunch of "if" statements and a switch without ranges? |
Yes, exactly. This feature request doesn't add any new expressive capability to switch, it is merely a space/time optimization. I agree that a "Big Bang" patch that updates all the things is impractical, so I'd suggest by enhancing the IR, but keeping all the (nontrivial) things that touch switch simple by lowering the rich representation to the old representation. As a follow-on, each place where this lowering happens can be eliminated, over time, and by experts on the different parts of the compiler. |
The current switch instruction uses an operand list with I am new here, so I might miss something extremely obvious, (and |
As mentioned in the comments before, the reason why If that is the case and a new instruction isn't really an option, I understand that it will be a lot of work on This might take a lot of time, but I don't think it is I will apologise once again, in case I've got something wrong (please correct me), and also for bad english, it isn't my first language. :) |
I agree that megapatches are very problematic, but I don't see how this requires one. There are straight-forward ways to phase this in as a series of incremental patches, using the existing SwitchInst.
I don't see how this would take a megapatch. |
I see, that will definitely help with the Just to clarify things for myself EliminateCaseRanges for now will |
|
What if to modify your approach as follows:
|
Thank you for explaining me what the EliminateCaseRanges What will it be replaced with once we start removing it |
The same for pass (when EliminateCaseRanges is a pass). In any option, I think that it should be removed only, after you added support to all existing passes. Then.. just remove this call from all places, and that's it. |
By adding support to all the passes, do you mean that all the passes will now be able to replace switch with ifs in case the range is greater than the threshold value that you earlier mentioned? Without using a call to EliminateCaseRanges? Or without that pass? |
As in every pass will be able to do what EleminateCaseRanges was doing? If this is the case then I guess keeping EliminateCaseRanges as a |
Please do not introduce #ifdef's in the codebase, there should be no need for them.
Correct.
I don't see a reason to worry about this unless some existing producer (e.g. clang's IRGen) would produce an if range. The simple way to phase that in is to just not have clang (or anything else) produce a switch range in a case that it would produce something smarter than the equivalent lowered switchinst. We can "take advantage" of case ranges when the whole compiler is more or less upgraded to deal with them. |
I think that perhaps we should consider two cases of EliminateCaseRanges use.
Am I right? |
Note quite. EliminateCaseRanges supposes that it produce code with NO case ranges. Whilst, if pass support case ranges, then that pass knows how to interpret it, and perhaps can even get some benefits out of case-ranges representation form. E.g. sparse constant propogation or loop unswitch pass could have some benefits in theory (basically all data flow analyzing passes could). Some targets could also support case ranges directly though. |
I have started working on adding case range support to the SwitchInst class. I am currently taking the approach taken by Stepan in the code he provided, I hope the approach is still relevant? |
Extended Description
The LLVM switch instruction should handle ranges of case values that go to the same destination without
enumerating all of them.
This would be good to do before 2.0, and would be a great project for someone who wants to learn/touch
more of LLVM.
-Chris
The text was updated successfully, but these errors were encountered: