After this function call, the LLVM IR would look like the following:
if (true) /* NonVersionedLoop */ else /* VersionedLoop */
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
User may need to create versioned loops of such structure to append more runtime checks conditions to the branch instruction.
Comment Actions
Are you planning on sharing a follow-up that makes use of the new functionality? That would be helpful to review this and the earlier patch, to make sure the new API is a good fit. Also, it would be good to have a way to test this.
Comment Actions
The design seems weird here.
I think it would be best to first see the intended use case.
Comment Actions
In terms of structuring of LoopVersioning, I think it would be great if the existing functionality could be refactored so that there is a set of helper functions that expose the functionality for re-use, rather than growing the existing LoopVersioning class.
Comment Actions
Hi @fhahn ,
Aside from adding helper functions, I think removing the assertion that "ensures RuntimeCheck is not a nullptr is necessary,
since in my use-case, the loop provided to LoopVersioning doesn't require any MemRuntimeCheck and SCEVRuntimeCheck but wants to clone the loop and setup a branch instruction for them.
Other than that I kept the interface and logic of versionLoop the same.
Thank you for reviewing this patch.
Comment Actions
Right, currently versionLoop roughly does the following things: 1) create memory runtime checks (if requested), 2) generate SCEV predicate checks (if requested) and 3) clone the loop and 4) branch to the right version depending on the generated runtime checks.
IIUC you only need 3). Personally it seems like extending versionLoop to only do 3) makes the interface more complicated than it needs to be. Instead, would it be possible to extract the code to do 3) into a utility function that returns the conditional branch to select the loop? Then you could use that in LoopIdiomRecognize directly.
| llvm/include/llvm/Transforms/Utils/LoopVersioning.h | ||
|---|---|---|
| 79 ↗ | (On Diff #354659) | dead code? (same for the other changes here) |
Comment Actions
IIUC you only need 3). Personally it seems like extending versionLoop to only do 3) makes the interface more complicated than it needs to be. Instead, would it be possible to extract the code to do 3) into a utility function that returns the conditional branch to select the loop? Then you could use that in LoopIdiomRecognize directly.
Hi @fhahn,
Sorry for the late reply.
I think I have made minimum change for versionLoop and only exposed utility functions (getRuntimeCheckBB and getRuntimeCheckBI) for use.
The usage of these two functions are inside D104636.
Since @Whitney have accepted it I think we can land this and proceed to the child patches and give LoopNestIdiomRecognize a try?
/// versionTopLoop - Create a fallback version the TopLoop
void LoopIdiomRecognize::versionTopLoop() {
const LoopAccessInfo LAI(TopLoop, SE, TLI, AA, DT, LI);
LoopVersioning LV(LAI, LAI.getRuntimePointerChecking()->getChecks(), TopLoop,
LI, DT, SE);
LV.versionLoopWithPlainRuntimeCheck();
RuntimeCheckBB = LV.getRuntimeCheckBB();
FallBackLoop = LV.getNonVersionedLoop();
}| llvm/include/llvm/Transforms/Utils/LoopVersioning.h | ||
|---|---|---|
| 79 ↗ | (On Diff #354659) | It is dead code right now but will be used in D104636. |
Comment Actions
FWIW I still think the interface should be adopted to make this more useable, rather than just adding on additional state that makes it even harder to make further changes. While the changes here are indeed a bit smaller than improving the interface, the additional state that needs maintained increases the complexity of the class even further, which has a cost down the road.
I *think* the patch should only be committed once D104636 has also been accepted; at the moment there's nothing to test the new functionality.
clang-format not found in user’s local PATH; not linting file.