This is an archive of the discontinued LLVM Phabricator instance.

Add `isRelExprOneOf` helper (alternative to D27145).
ClosedPublic

Authored by silvas on Nov 27 2016, 8:28 PM.

Details

Summary

Additionally, this uses a fast implementation to do the test, significantly reducing the amount of branching in this hot path of the relocation handling code.

Thanks to Joerg for suggesting this simple way to do this that should still retain most of the performance advantages of the approach in D27145.

Diff Detail

Event Timeline

silvas updated this revision to Diff 79366.Nov 27 2016, 8:28 PM
silvas retitled this revision from to Add `isRelExprOneOf` helper (alternative to D27145)..
silvas updated this object.
silvas added reviewers: ruiu, rafael, joerg.
silvas added a subscriber: llvm-commits.
grimar added a subscriber: grimar.Nov 27 2016, 11:43 PM
joerg edited edge metadata.Nov 28 2016, 2:31 AM

Otherwise, LGTM.

ELF/Relocations.h
81

Small item, but please add a static assert that 0 <= Head < 64 here.

90

...and a run-time assert for for 0<=Expr < 64 here.

ruiu accepted this revision.Nov 28 2016, 7:09 AM
ruiu edited edge metadata.

Ah, so there's a new patch. This looks nice. LGTM.

Before submitting, please update the commit message so that you'll keep the message you wrote for https://reviews.llvm.org/D27145 in our commit history.

ELF/Relocations.h
78

nit: add a blank line.

90

Maybe we should instead define R_END at end of RelExpr and statically assert R_END < 64;

This revision is now accepted and ready to land.Nov 28 2016, 7:09 AM
emaste added a subscriber: emaste.Nov 28 2016, 7:30 AM
joerg added inline comments.Nov 28 2016, 7:33 AM
ELF/Relocations.h
90

One motivation for doing the asserts here is that there is a good chance it will get copied to other places. So getting it right the first time helps avoiding surprises in other places where the constraints are not that obvious.

silvas updated this revision to Diff 79704.Nov 29 2016, 9:58 PM
silvas edited edge metadata.

Sadly, it seems like static_assert can't be used on constexpr function arguments, so I've had to rewrite this to pass the set of RelExprs as template arguments. Though the implementation of the mask construction becomes slightly larger, I actually like the template argument approach because the call sites of isRelExprOneOf have the the value to check in the runtime argument list separate from the set of values to check against (which are now in the template arguments).

Just posting this explicitly so that if one of you feels strongly I can go back to the original version with constexpr and use an R_END technique to static assert (that gives a bit less protection though), but the fact that I think the callsites are cleaner for the template version seems worth it to stick with the template. Please let me know if this is good or if I should go back to the original and use R_END.

ruiu added a comment.Nov 29 2016, 10:55 PM

I agree that at the call side this looks better, but after reviewing the previous one again, I think I still prefer the previous one, because recursive templates are more complicated than the original simple-minded constexpr containing a for-loop. In practice, static_assert'ing on R_END would be enough as a protection.

In D27156#608816, @ruiu wrote:

I agree that at the call side this looks better, but after reviewing the previous one again, I think I still prefer the previous one, because recursive templates are more complicated than the original simple-minded constexpr containing a for-loop. In practice, static_assert'ing on R_END would be enough as a protection.

Unfortunately the R_END approach causes "unhandled case in switch" warnings in getSymVA. What do you think? I like the safety of these switch warnings.

ELF/Relocations.h
81

Sadly it seems like static_assert is not allowed inside constexpr functions :(

silvas updated this revision to Diff 79706.Nov 30 2016, 12:39 AM

Somewhat simpler template version. Let me know what you think. I think the improved clarity at the call site is worth it.

the original simple-minded constexpr containing a for-loop

There wasn't a for loop. It was still recursive in the constexpr case.

Thinking about this for a little bit, I actually like the extra clarity at the use site for the template approach. If you don't mind, I would like to keep it that way.
The templates are not any more complicated (they are both a recursive approach). The templates are slightly more lines of code, but the conceptual complexity is still the same.

silvas updated this revision to Diff 79707.Nov 30 2016, 12:41 AM

Small fixup (delete some code that I forgot to remove in the last patch).

joerg accepted this revision.Nov 30 2016, 3:16 AM
joerg edited edge metadata.

This looks fine thanks.

This revision was automatically updated to reflect the committed changes.